Re: [pmg-devel] [PATCH pmg-api 2/6] config: add plugin system for realms & add openid type realms

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 02/04/2024 um 13:27 schrieb Markus Frank:
> To differentiate between usernames, the realm is also stored in the
> user.conf file. Old config file syntax can be read, but will be
> overwritten after a change.
> 
> Utils generates a list of valid realm names, including any newly added realms,
> to ensure proper validation of a specified realm name.

you might also talk a bit here about that this is adapted over from PVE,
how/why the static/limited realms worked earlier in PMG, how that is changed
and all that, having some background and reference can only help

> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/Makefile               |   3 +
>  src/PMG/AccessControl.pm   |  33 +++++
>  src/PMG/Auth/OpenId.pm     |  99 ++++++++++++++
>  src/PMG/Auth/PMG.pm        |  28 ++++
>  src/PMG/Auth/Plugin.pm     | 269 +++++++++++++++++++++++++++++++++++++
>  src/PMG/RESTEnvironment.pm |  14 ++
>  src/PMG/UserConfig.pm      |  26 ++--
>  src/PMG/Utils.pm           |  24 +++-
>  8 files changed, 482 insertions(+), 14 deletions(-)
>  create mode 100755 src/PMG/Auth/OpenId.pm
>  create mode 100755 src/PMG/Auth/PMG.pm
>  create mode 100755 src/PMG/Auth/Plugin.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index 8e49a10..407cc4a 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -164,6 +164,9 @@ LIBSOURCES =				\
>  	PMG/API2/ACMEPlugin.pm		\
>  	PMG/API2/NodeConfig.pm		\
>  	PMG/API2.pm			\
> +	PMG/Auth/Plugin.pm		\
> +	PMG/Auth/OpenId.pm		\
> +	PMG/Auth/PMG.pm


nit: missing trailing backslash (so any new addition doesn't need to touch
existing lines)

>  
>  SOURCES = ${LIBSOURCES} ${CLI_BINARIES} ${TEMPLATES_FILES} ${CONF_MANS} ${CLI_MANS} ${SERVICE_MANS} ${SERVICE_UNITS} ${TIMER_UNITS} pmg-sources.list pmg-initramfs.conf
>  
> diff --git a/src/PMG/AccessControl.pm b/src/PMG/AccessControl.pm
> index e12d7cf..0ab4dfa 100644
> --- a/src/PMG/AccessControl.pm
> +++ b/src/PMG/AccessControl.pm
> @@ -7,12 +7,21 @@ use Authen::PAM;
>  use PVE::Tools;
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Exception qw(raise raise_perm_exc);
> +use PVE::Tools qw(lock_file);

Tools is already used three lines above this, add the import of lock_file there. 

>  
>  use PMG::UserConfig;
>  use PMG::LDAPConfig;
>  use PMG::LDAPSet;
>  use PMG::TFAConfig;
>  
> +use PMG::Auth::Plugin;
> +use PMG::Auth::OpenId;
> +use PMG::Auth::PMG;
> +
> +PMG::Auth::OpenId->register();
> +PMG::Auth::PMG->register();
> +PMG::Auth::Plugin->init();
> +
>  sub normalize_path {
>      my $path = shift;
>  
> @@ -38,6 +47,8 @@ sub authenticate_user : prototype($$$) {
>  
>      ($username, $ruid, $realm) = PMG::Utils::verify_username($username);
>  
> +    my $valid_pmg_realms = PMG::Utils::valid_pmg_realms();
> +    my $realm_list = join('|', @$valid_pmg_realms);
>      if ($realm eq 'pam') {
>  	die "invalid pam user (only root allowed)\n" if $ruid ne 'root';
>  	authenticate_pam_user($ruid, $password);
> @@ -53,6 +64,7 @@ sub authenticate_user : prototype($$$) {
>  	    return ($pmail . '@quarantine', undef);
>  	}
>  	die "ldap login failed\n";
> +    } elsif ($realm =~ m!(${realm_list})!) {

stumbling upon empty branches lets one always wonder what it's about, so
adding some comment here stating the reason this is done would be good.

>      } else {
>  	die "no such realm '$realm'\n";
>      }
> @@ -79,6 +91,8 @@ sub set_user_password {
>      
>      ($username, $ruid, $realm) = PMG::Utils::verify_username($username);
>  
> +    my $valid_pmg_realms = PMG::Utils::valid_pmg_realms();
> +    my $realm_list = join('|', @$valid_pmg_realms);
>      if ($realm eq 'pam') {
>  	die "invalid pam user (only root allowed)\n" if $ruid ne 'root';
>  
> @@ -92,6 +106,7 @@ sub set_user_password {
>  
>      } elsif ($realm eq 'pmg') {
>  	PMG::UserConfig->set_user_password($username, $password);
> +    } elsif ($realm =~ m!(${realm_list})!) {

same here

>      } else {
>  	die "no such realm '$realm'\n";
>      }
> @@ -106,6 +121,8 @@ sub check_user_enabled {
>  
>      ($username, $ruid, $realm) = PMG::Utils::verify_username($username, 1);
>  
> +    my $valid_pmg_realms = PMG::Utils::valid_pmg_realms();
> +    my $realm_list = join('|', @$valid_pmg_realms);
>      if ($realm && $ruid) {
>  	if ($realm eq 'pam') {
>  	    return 'root' if $ruid eq 'root';
> @@ -115,6 +132,10 @@ sub check_user_enabled {
>  	    return $data->{role} if $data && $data->{enable};
>  	} elsif ($realm eq 'quarantine') {
>  	    return 'quser';
> +	} elsif ($realm =~ m!(${realm_list})!) {
> +	    my $usercfg = PMG::UserConfig->new();
> +	    my $data = $usercfg->lookup_user_data($username, $noerr);
> +	    return $data->{role} if $data && $data->{enable};
>  	}
>      }
>  
> @@ -123,6 +144,18 @@ sub check_user_enabled {
>      return undef;
>  }
>  
> +sub check_user_exist {
> +    my ($usercfg, $username, $noerr) = @_;
> +
> +    $username = PMG::Utils::verify_username($username, $noerr);
> +    return undef if !$username;
> +    return $usercfg->{$username} if $usercfg && $usercfg->{$username};
> +
> +    die "no such user ('$username')\n" if !$noerr;
> +
> +    return undef;
> +}
> +
>  sub authenticate_pam_user {
>      my ($username, $password) = @_;
>  
> diff --git a/src/PMG/Auth/OpenId.pm b/src/PMG/Auth/OpenId.pm
> new file mode 100755
> index 0000000..44fe0fe
> --- /dev/null
> +++ b/src/PMG/Auth/OpenId.pm

IIRC you took this from PVE, but maybe we should name this OIDC.pm (or, for
verbosity, OpenIDConnect.pm) here, as it's not really the (rather broken) OpenID,
but the successor OpenID Connect. Having this distinction encoded in the
filename would not hurt and might potentially soothe some paranoid admins
that had to much contact with security stuff.

> @@ -0,0 +1,99 @@
> +package PMG::Auth::OpenId;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Tools;
> +use PMG::Auth::Plugin;
> +
> +use base qw(PMG::Auth::Plugin);
> +
> +sub type {
> +    return 'openid';

same her w.r.t "OpenID" vs "OpenID Connect" (or it's abbreviation "OIDC").

> +}
> +
> +sub properties {
> +    return {
> +	"issuer-url" => {
> +	    description => "OpenID Issuer Url",
> +	    type => 'string',
> +	    maxLength => 256,
> +	},
> +	"client-id" => {
> +	    description => "OpenID Client ID",
> +	    type => 'string',
> +	    maxLength => 256,
> +	},
> +	"client-key" => {
> +	    description => "OpenID Client Key",
> +	    type => 'string',
> +	    optional => 1,
> +	    maxLength => 256,
> +	},
> +	autocreate => {
> +	    description => "Automatically create users if they do not exist.",
> +	    optional => 1,
> +	    type => 'boolean',
> +	    default => 0,
> +	},
> +	"username-claim" => {
> +	    description => "OpenID claim used to generate the unique username.",
> +	    type => 'string',
> +	    optional => 1,
> +	},
> +	prompt => {
> +	    description => "Specifies whether the Authorization Server prompts the End-User for"
> +	        ." reauthentication and consent.",
> +	    type => 'string',
> +	    pattern => '(?:none|login|consent|select_account|\S+)', # \S+ is the extension variant
> +	    optional => 1,
> +	},
> +	scopes => {
> +	    description => "Specifies the scopes (user details) that should be authorized and"
> +	        ." returned, for example 'email' or 'profile'.",
> +	    type => 'string', # format => 'some-safe-id-list', # FIXME: TODO
> +	    default => "email profile",
> +	    optional => 1,
> +	},
> +	'acr-values' => {
> +	    description => "Specifies the Authentication Context Class Reference values that the"
> +		."Authorization Server is being requested to use for the Auth Request.",
> +	    type => 'string', # format => 'some-safe-id-list', # FIXME: TODO
> +	    optional => 1,
> +	},
> +	default => {
> +	    description => "Use this as default realm",
> +	    type => 'boolean',
> +	    optional => 1,
> +	},
> +	comment => {
> +	    description => "Description.",
> +	    type => 'string',
> +	    optional => 1,
> +	    maxLength => 4096,
> +	},
> +   };
> +}
> +
> +sub options {
> +    return {

small nit: here you use double-quotes for keys with a minus, but in the property
schema definition you use single-quotes for them – might also use them here.

> +	"issuer-url" => {},
> +	"client-id" => {},
> +	"client-key" => { optional => 1 },
> +	autocreate => { optional => 1 },
> +	"username-claim" => { optional => 1, fixed => 1 },
> +	prompt => { optional => 1 },
> +	scopes => { optional => 1 },
> +	"acr-values" => { optional => 1 },
> +	default => { optional => 1 },
> +	comment => { optional => 1 },
> +    };
> +}
> +
> +sub authenticate_user {
> +    my ($class, $config, $realm, $username, $password) = @_;
> +
> +    die "OpenID realm does not allow password verification.\n";
> +}
> +
> +1;
> diff --git a/src/PMG/Auth/PMG.pm b/src/PMG/Auth/PMG.pm
> new file mode 100755
> index 0000000..0eb136c
> --- /dev/null
> +++ b/src/PMG/Auth/PMG.pm
> @@ -0,0 +1,28 @@
> +package PMG::Auth::PMG;
> +
> +use strict;
> +use warnings;
> +
> +use PMG::Auth::Plugin;
> +
> +use base qw(PMG::Auth::Plugin);
> +
> +sub type {
> +    return 'pmg';
> +}
> +
> +sub properties {
> +    return {
> +	tfa => PVE::JSONSchema::get_standard_option('tfa'),
> +    };
> +}
> +
> +sub options {
> +    return {
> +	default => { optional => 1 },
> +	comment => { optional => 1 },
> +	tfa => { optional => 1 },
> +    };
> +}
> +
> +1;
> diff --git a/src/PMG/Auth/Plugin.pm b/src/PMG/Auth/Plugin.pm
> new file mode 100755
> index 0000000..a7ea596
> --- /dev/null
> +++ b/src/PMG/Auth/Plugin.pm
> @@ -0,0 +1,269 @@
> +package PMG::Auth::Plugin;
> +
> +use strict;
> +use warnings;
> +
> +use Digest::SHA;
> +use Encode;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::SectionConfig;
> +use PVE::Tools;
> +use PVE::INotify;
> +use PMG::Utils;

nit: group and sort

> +
> +use base qw(PVE::SectionConfig);
> +
> +my $domainconfigfile = "realms.cfg";
> +my $lockfile = "/var/lock/realms.lck";
> +
> +sub read_realms_conf {
> +    my ($filename, $fh) = @_;
> +
> +    my $raw;
> +    $raw = do { local $/ = undef; <$fh> } if defined($fh);
> +
> +    return  PMG::Auth::Plugin->parse_config($filename, $raw);
> +}
> +
> +sub write_realms_conf {
> +    my ($filename, $fh, $cfg) = @_;
> +
> +    my $raw = PMG::Auth::Plugin->write_config($filename, $cfg);
> +
> +    PVE::Tools::safe_print($filename, $fh, $raw);
> +}
> +
> +PVE::INotify::register_file(
> +    $domainconfigfile,
> +    "/etc/pmg/realms.cfg",
> +    \&read_realms_conf,
> +    \&write_realms_conf,
> +    undef,
> +    always_call_parser => 1

nit, missing trailing comma

> +);
> +
> +sub lock_domain_config {
> +    my ($code, $errmsg) = @_;
> +
> +    PVE::Tools::lock_file($lockfile, undef, $code);
> +    my $err = $@;
> +    if ($err) {

nit: could avoid an extra line:

if (my $err = $@) {
    ...

> +	$errmsg ? die "$errmsg: $err" : die $err;
> +    }
> +}
> +
> +my $realm_regex = qr/[A-Za-z][A-Za-z0-9\.\-_]+/;
> +
> +sub pmg_verify_realm {
> +    my ($realm, $noerr) = @_;
> +
> +    if ($realm !~ m/^${realm_regex}$/) {
> +	return undef if $noerr;
> +	die "value does not look like a valid realm\n";
> +    }
> +    return $realm;
> +}
> +
> +my $remove_options = "(?:acl|properties|entry)";
> +
> +PVE::JSONSchema::register_standard_option('sync-scope', {
> +    description => "Select what to sync.",
> +    type => 'string',
> +    enum => [qw(users groups both)],
> +    optional => '1',
> +});
> +
> +PVE::JSONSchema::register_standard_option('sync-remove-vanished', {
> +    description => "A semicolon-seperated list of things to remove when they or the user"
> +	." vanishes during a sync. The following values are possible: 'entry' removes the"
> +	." user/group when not returned from the sync. 'properties' removes the set"
> +	." properties on existing user/group that do not appear in the source (even custom ones)."
> +	." 'acl' removes acls when the user/group is not returned from the sync."
> +	." Instead of a list it also can be 'none' (the default).",
> +    type => 'string',
> +    default => 'none',
> +    typetext => "([acl];[properties];[entry])|none",
> +    pattern => "(?:(?:$remove_options\;)*$remove_options)|none",
> +    optional => '1',

should we move this plus the other copied formats/regexs/verifiers/.. to a
common-schema package (pve-common would be due for a split since quite a
while already anyway ^^).

You'd not have to do all the work for that though, moving the definitions to
pve-common in a new module (e.g., PVE::Schema::Auth) would allow us to reuse
it now in PVE and PMG without to much work and allow to split this later out
into a new package relatively easily.

> +});
> +
> +my $realm_sync_options_desc = {
> +    scope => get_standard_option('sync-scope'),
> +    'remove-vanished' => get_standard_option('sync-remove-vanished'),
> +    'enable-new' => {
> +	description => "Enable newly synced users immediately.",
> +	type => 'boolean',
> +	default => '1',
> +	optional => '1',
> +    },
> +};
> +PVE::JSONSchema::register_standard_option('realm-sync-options', $realm_sync_options_desc);
> +PVE::JSONSchema::register_format('realm-sync-options', $realm_sync_options_desc);
> +
> +my $tfa_format = {
> +    type => {
> +        description => "The type of 2nd factor authentication.",
> +        format_description => 'TFATYPE',
> +        type => 'string',
> +        enum => [qw(oath)],
> +    },
> +    digits => {
> +        description => "TOTP digits.",
> +        format_description => 'COUNT',
> +        type => 'integer',
> +        minimum => 6, maximum => 8,
> +        default => 6,
> +        optional => 1,
> +    },
> +    step => {
> +        description => "TOTP time period.",
> +        format_description => 'SECONDS',
> +        type => 'integer',
> +        minimum => 10,
> +        default => 30,
> +        optional => 1,
> +    },
> +};
> +
> +PVE::JSONSchema::register_format('pmg-tfa-config', $tfa_format);
> +
> +PVE::JSONSchema::register_standard_option('tfa', {
> +    description => "Use Two-factor authentication.",
> +    type => 'string', format => 'pmg-tfa-config',
> +    optional => 1,
> +    maxLength => 128,
> +});
> +
> +sub parse_tfa_config {
> +    my ($data) = @_;
> +
> +    return PVE::JSONSchema::parse_property_string($tfa_format, $data);
> +}
> +
> +my $defaultData = {
> +    propertyList => {
> +	type => { description => "Realm type." },
> +	realm => get_standard_option('realm'),
> +    },
> +};
> +
> +sub private {
> +    return $defaultData;
> +}
> +
> +sub parse_section_header {
> +    my ($class, $line) = @_;
> +
> +    if ($line =~ m/^(\S+):\s*(\S+)\s*$/) {
> +	my ($type, $realm) = (lc($1), $2);
> +	my $errmsg = undef; # set if you want to skip whole section
> +	eval { pmg_verify_realm($realm); };
> +	$errmsg = $@ if $@;
> +	my $config = {}; # to return additional attributes
> +	return ($type, $realm, $errmsg, $config);
> +    }
> +    return undef;
> +}
> +
> +sub parse_config {
> +    my ($class, $filename, $raw) = @_;
> +
> +    my $cfg = $class->SUPER::parse_config($filename, $raw);
> +
> +    my $default;
> +    foreach my $realm (keys %{$cfg->{ids}}) {
> +	my $data = $cfg->{ids}->{$realm};
> +	# make sure there is only one default marker
> +	if ($data->{default}) {
> +	    if ($default) {
> +		delete $data->{default};
> +	    } else {
> +		$default = $realm;
> +	    }
> +	}
> +
> +	if ($data->{comment}) {
> +	    $data->{comment} = PVE::Tools::decode_text($data->{comment});
> +	}
> +
> +    }
> +
> +    # add default domains
> +    $cfg->{ids}->{pmg}->{type} = 'pmg'; # force type
> +    $cfg->{ids}->{pmg}->{comment} = "Proxmox Mail Gateway authentication server"
> +	if !$cfg->{ids}->{pmg}->{comment};
> +
> +    return $cfg;
> +};
> +
> +sub write_config {
> +    my ($class, $filename, $cfg) = @_;
> +
> +    foreach my $realm (keys %{$cfg->{ids}}) {
> +	my $data = $cfg->{ids}->{$realm};
> +	if ($data->{comment}) {
> +	    $data->{comment} = PVE::Tools::encode_text($data->{comment});
> +	}
> +    }
> +
> +    $class->SUPER::write_config($filename, $cfg);
> +}
> +
> +sub authenticate_user {
> +    my ($class, $config, $realm, $username, $password) = @_;
> +
> +    die "overwrite me";
> +}
> +
> +sub store_password {
> +    my ($class, $config, $realm, $username, $password) = @_;
> +
> +    my $type = $class->type();
> +
> +    die "can't set password on auth type '$type'\n";
> +}
> +
> +sub delete_user {
> +    my ($class, $config, $realm, $username) = @_;
> +
> +    # do nothing by default
> +}
> +
> +# called during addition of realm (before the new domain config got written)
> +# `password` is moved to %param to avoid writing it out to the config
> +# die to abort addition if there are (grave) problems
> +# NOTE: runs in a domain config *locked* context
> +sub on_add_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
> +# called during domain configuration update (before the updated domain config got
> +# written). `password` is moved to %param to avoid writing it out to the config
> +# die to abort the update if there are (grave) problems
> +# NOTE: runs in a domain config *locked* context
> +sub on_update_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
> +# called during deletion of realms (before the new domain config got written)
> +# and if the activate check on addition fails, to cleanup all storage traces
> +# which on_add_hook may have created.
> +# die to abort deletion if there are (very grave) problems
> +# NOTE: runs in a domain config *locked* context
> +sub on_delete_hook {
> +    my ($class, $realm, $config) = @_;
> +    # do nothing by default
> +}
> +
> +# called during addition and updates of realms (before the new domain config gets written)
> +# die to abort addition/update in case the connection/bind fails
> +# NOTE: runs in a domain config *locked* context
> +sub check_connection {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
> +1;
> diff --git a/src/PMG/RESTEnvironment.pm b/src/PMG/RESTEnvironment.pm
> index 3875720..f6ff449 100644
> --- a/src/PMG/RESTEnvironment.pm
> +++ b/src/PMG/RESTEnvironment.pm
> @@ -88,6 +88,20 @@ sub get_role {
>      return $self->{role};
>  }
>  
> +sub check_user_enabled {
> +    my ($self, $user, $noerr) = @_;
> +
> +    my $cfg = $self->{usercfg};
> +    return PMG::AccessControl::check_user_enabled($cfg, $user, $noerr);
> +}
> +
> +sub check_user_exist {
> +    my ($self, $user, $noerr) = @_;
> +
> +    my $cfg = $self->{usercfg};
> +    return PMG::AccessControl::check_user_exist($cfg, $user, $noerr);
> +}
> +
>  sub check_node_is_master {
>      my ($self, $noerr);
>  
> diff --git a/src/PMG/UserConfig.pm b/src/PMG/UserConfig.pm
> index b9a83a7..8fb93c9 100644
> --- a/src/PMG/UserConfig.pm
> +++ b/src/PMG/UserConfig.pm
> @@ -80,7 +80,7 @@ my $schema = {
>  	realm => {
>  	    description => "Authentication realm.",
>  	    type => 'string',
> -	    enum => ['pam', 'pmg'],
> +	    format => 'pmg-realm',
>  	    default => 'pmg',
>  	    optional => 1,
>  	},
> @@ -219,10 +219,13 @@ sub read_user_conf {
>                 (?<keys>(?:[^:]*)) :
>                 $/x
>  	    ) {
> +		my @username_parts = split('@', $+{userid});

Yuck on the pre-existing `$+{userid}` for , but oh well..

https://perldoc.perl.org/variables/$+

> +		my $username = $username_parts[0];
> +		my $realm = defined($username_parts[1]) ? $username_parts[1] : "pmg";
>  		my $d = {
> -		    username => $+{userid},
> -		    userid => $+{userid} . '@pmg',
> -		    realm => 'pmg',
> +		    username => $username,
> +		    userid => $username . '@' . $realm,
> +		    realm => $realm,
>  		    enable => $+{enable} || 0,
>  		    expire => $+{expire} || 0,
>  		    role => $+{role},
> @@ -235,8 +238,9 @@ sub read_user_conf {
>  		eval {
>  		    $verify_entry->($d);
>  		    $cfg->{$d->{userid}} = $d;
> -		    die "role 'root' is reserved\n"
> -			if $d->{role} eq 'root' && $d->{userid} ne 'root@pmg';
> +		    if ($d->{role} eq 'root' && $d->{userid} !~ /^root@(pmg|pam)$/) {
> +			die "role 'root' is reserved\n";
> +		    }
>  		};
>  		if (my $err = $@) {
>  		    warn "$filename: $err";
> @@ -274,22 +278,24 @@ sub write_user_conf {
>  	$verify_entry->($d);
>  	$cfg->{$d->{userid}} = $d;
>  
> +	my $valid_pmg_realms = PMG::Utils::valid_pmg_realms();
> +	my $realm_list = join('|', @$valid_pmg_realms);
>  	if ($d->{userid} ne 'root@pam') {
>  	    die "role 'root' is reserved\n" if $d->{role} eq 'root';
>  	    die "unable to add users for realm '$d->{realm}'\n"
> -		if $d->{realm} && $d->{realm} ne 'pmg';
> +		if $d->{realm} && $d->{realm} !~ m!(${realm_list})!;
>  	}
>  
>  	my $line;
>  
>  	if ($userid eq 'root@pam') {
> -	    $line = 'root:';
> +	    $line = 'root@pam:';
>  	    $d->{crypt_pass} = '',
>  	    $d->{expire} = '0',
>  	    $d->{role} = 'root';
>  	} else {
> -	    next if $userid !~ m/^(?<username>.+)\@pmg$/;
> -	    $line = "$+{username}:";
> +	    next if $userid !~ m/^(?<username>.+)\@(${realm_list})$/;
> +	    $line = "$d->{userid}:";
>  	}
>  
>  	for my $k (qw(enable expire crypt_pass role email firstname lastname keys)) {
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index 5d9ded4..e173894 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -49,12 +49,27 @@ postgres_admin_cmd
>  try_decode_utf8
>  );
>  
> -my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
> +my $user_regex = qr![^\s:/]+!;
> +
> +sub valid_pmg_realms {
> +    my $cfg = PVE::INotify::read_file('realms.cfg');
> +    my $ids = $cfg->{ids};
> +    my $realms = [];
> +    my @list = keys %{$ids};
> +    for my $realm (@list) {
> +	push @$realms, $realm;
> +    }

This can be quite a bit reduced to:

my $realms = [ sort keys $cfg->{ids}->%* ];

> +    push @$realms, 'pam';
> +    push @$realms, 'quarantine';

can be fine like this, but FWIW, you could push multiple values at once or
even further reduce this to:

return [ 'pam', 'quarantine', keys $cfg->{ids}->%* ];

But, FWICT you always use this in a regex to test if something is a valid realm, so
why stop bothering with this and rather either:

- add (or transform this into) a valid_pmg_realm_regex method
- change it to a `is_valid_realm` method that takes the to-check realm value as param
  and then implement that like:
  sub is_valid_realm {
      my ($realm) = @_;
      return 0 if !$realm;
      return 1 if $realm eq 'pam' || $realm eq 'quarantine'; # built-in ones

      my $cfg = PVE::INotify::read_file('realms.cfg');
      return exists($cfg->{ids}->{$realm}) ? 1 : 0;
  }
  would be faster for the built-in ones (no config parse) and avoid some array
  creation from a hash-map only to join into a regex by simply using the original
  hash-map directly for all our use case.

> +    return $realms;
> +}
> +
> +PVE::JSONSchema::register_format('pmg-realm', \&valid_pmg_realms);
>  
>  PVE::JSONSchema::register_standard_option('realm', {
>      description => "Authentication domain ID",
>      type => 'string',
> -    enum => $valid_pmg_realms,
> +    format => 'pmg-realm',
>      maxLength => 32,
>  });
>  
> @@ -82,7 +97,7 @@ sub verify_username {
>  	die "user name '$username' is too short\n" if !$noerr;
>  	return undef;
>      }
> -    if ($len > 64) {
> +    if ($len > 128) {
>  	die "user name '$username' is too long ($len > 64)\n" if !$noerr;

you missed updating the error message, so this would be rather confusing if one runs
into it.

>  	return undef;
>      }
> @@ -90,8 +105,9 @@ sub verify_username {
>      # we only allow a limited set of characters. Colons aren't allowed, because we store usernames
>      # with colon separated lists! slashes aren't allowed because it is used as pve API delimiter
>      # also see "man useradd"
> +    my $valid_pmg_realms = valid_pmg_realms();

does this need the realm list here, i.e., isn't the existence of the realm checked
already elsewhere already so that we can use some general [a-z]+ for that part in
the regex here?

>      my $realm_list = join('|', @$valid_pmg_realms);
> -    if ($username =~ m!^([^\s:/]+)\@(${realm_list})$!) {
> +    if ($username =~ m!^(${user_regex})\@(${realm_list})$!) {
>  	return wantarray ? ($username, $1, $2) : $username;
>      }
>