Re: [pmg-devel] [PATCH pmg-api v6 4/12] config: add plugin system for authentication realms

[Stoiko Ivanov <s.ivanov@proxmox.com>]

On Tue, 25 Feb 2025 14:36:11 +0100
Markus Frank <m.frank@proxmox.com> wrote:

> 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.
did not get around to check this for you v5 - after seeing Fabian's
feedback in
https://lore.proxmox.com/pmg-devel/Z7YlS-KYHpQMG6Nt@rosa.proxmox.com/T/#ma1084dabd040ce120f0fe831b5748eb931a21982
I wondered if 'foo@bar' is a valid username in PMG - the answer is sadly:
only partially - you can even create this in the GUI successfully, and it
ends up in user.conf - but reading the entry fails with:
```
verify entry failed
username: value does not match the regex pattern
```

we should fix this and forbid creating such users in general, but at least
we can assume that such users are not in use and can keep the 'realm' next
to the username as 'username@realm' as we do for PVE (and for PBS,
discounting for the different format)

issue stems from having 'userid' (root@pam), 'username' ('root') as
2 separate standard_options - userid only enforces a length-boundary and
forbids ':|/', while 'username' forbids '@' in addition.

I'll look into preparing an independent patch for that part of validation.


> 
> This is a carryover from PVE. Previously there was no realm for a
> username. Now the realm is also stored after an @ sign in user.conf.
This part here is not true for this version of the patch - and the notes
below, that do mention it would not end up in git.

> 
> Utils generates a list of valid realm names, including any newly added
> realms, to ensure proper validation of a specified realm name.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> v6: 
> * instead of appending @realm to the username, the realm is now saved
>  after the other properties in user.conf.
> * authenticate user and store password inside plugin like it is in PVE
> * renamed $domainconfigfile to $realm_cfg_id
> 
>  src/Makefile               |   3 +
>  src/PMG/API2/Users.pm      |   1 +
>  src/PMG/AccessControl.pm   |  38 +++++++
>  src/PMG/Auth/PAM.pm        |  22 ++++
>  src/PMG/Auth/PMG.pm        |  39 +++++++
>  src/PMG/Auth/Plugin.pm     | 203 +++++++++++++++++++++++++++++++++++++
>  src/PMG/RESTEnvironment.pm |  14 +++
>  src/PMG/UserConfig.pm      |  24 +++--
>  src/PMG/Utils.pm           |  29 ++++--
>  9 files changed, 359 insertions(+), 14 deletions(-)
>  create mode 100755 src/PMG/Auth/PAM.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 1232880..659a666 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -166,6 +166,9 @@ LIBSOURCES =				\
>  	PMG/API2/ACMEPlugin.pm		\
>  	PMG/API2/NodeConfig.pm		\
>  	PMG/API2.pm			\
> +	PMG/Auth/Plugin.pm		\
> +	PMG/Auth/PAM.pm			\
> +	PMG/Auth/PMG.pm			\
>  
>  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/API2/Users.pm b/src/PMG/API2/Users.pm
> index 99af38d..42c82b7 100644
> --- a/src/PMG/API2/Users.pm
> +++ b/src/PMG/API2/Users.pm
> @@ -124,6 +124,7 @@ __PACKAGE__->register_method ({
>  	    $entry->{enable} //= 0;
>  	    $entry->{expire} //= 0;
>  	    $entry->{role} //= 'audit';
> +	    $entry->{realm} //= 'pmg';
>  
>  	    $cfg->{$param->{userid}} = $entry;
>  
> diff --git a/src/PMG/AccessControl.pm b/src/PMG/AccessControl.pm
> index e12d7cf..78105c0 100644
> --- a/src/PMG/AccessControl.pm
> +++ b/src/PMG/AccessControl.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use Authen::PAM;
>  
>  use PVE::Tools;
> +use PVE::INotify;
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Exception qw(raise raise_perm_exc);
>  
> @@ -13,6 +14,14 @@ use PMG::LDAPConfig;
>  use PMG::LDAPSet;
>  use PMG::TFAConfig;
>  
> +use PMG::Auth::Plugin;
> +use PMG::Auth::PAM;
> +use PMG::Auth::PMG;
> +
> +PMG::Auth::PAM->register();
> +PMG::Auth::PMG->register();
> +PMG::Auth::Plugin->init();
> +
>  sub normalize_path {
>      my $path = shift;
>  
> @@ -38,6 +47,7 @@ sub authenticate_user : prototype($$$) {
>  
>      ($username, $ruid, $realm) = PMG::Utils::verify_username($username);
>  
> +    my $realm_regex = PMG::Utils::valid_pmg_realm_regex();
>      if ($realm eq 'pam') {
>  	die "invalid pam user (only root allowed)\n" if $ruid ne 'root';
>  	authenticate_pam_user($ruid, $password);
> @@ -53,6 +63,11 @@ sub authenticate_user : prototype($$$) {
>  	    return ($pmail . '@quarantine', undef);
>  	}
>  	die "ldap login failed\n";
> +    } elsif ($realm =~ m!(${realm_regex})!) {
> +	my $realm_cfg = PVE::INotify::read_file(PMG::Auth::Plugin->realm_cfg_id());
> +	my $cfg = $realm_cfg->{ids}->{$realm};
> +	my $plugin = PMG::Auth::Plugin->lookup($cfg->{type});
> +	$plugin->authenticate_user($cfg, $realm, $ruid, $password);
>      } else {
>  	die "no such realm '$realm'\n";
>      }
> @@ -79,6 +94,7 @@ sub set_user_password {
>      
>      ($username, $ruid, $realm) = PMG::Utils::verify_username($username);
>  
> +    my $realm_regex = PMG::Utils::valid_pmg_realm_regex();
>      if ($realm eq 'pam') {
>  	die "invalid pam user (only root allowed)\n" if $ruid ne 'root';
>  
> @@ -92,6 +108,11 @@ sub set_user_password {
>  
>      } elsif ($realm eq 'pmg') {
>  	PMG::UserConfig->set_user_password($username, $password);
> +    } elsif ($realm =~ m!(${realm_regex})!) {
> +	my $realm_cfg = PVE::INotify::read_file(PMG::Auth::Plugin->realm_cfg_id());
> +	my $cfg = $realm_cfg->{ids}->{$realm};
> +	my $plugin = PMG::Auth::Plugin->lookup($cfg->{type});
> +	$plugin->store_password($cfg, $realm, $username, $password);
>      } else {
>  	die "no such realm '$realm'\n";
>      }
> @@ -106,6 +127,7 @@ sub check_user_enabled {
>  
>      ($username, $ruid, $realm) = PMG::Utils::verify_username($username, 1);
>  
> +    my $realm_regex = PMG::Utils::valid_pmg_realm_regex();
>      if ($realm && $ruid) {
>  	if ($realm eq 'pam') {
>  	    return 'root' if $ruid eq 'root';
> @@ -115,6 +137,10 @@ sub check_user_enabled {
>  	    return $data->{role} if $data && $data->{enable};
>  	} elsif ($realm eq 'quarantine') {
>  	    return 'quser';
> +	} elsif ($realm =~ m!(${realm_regex})!) {
> +	    my $usercfg = PMG::UserConfig->new();
> +	    my $data = $usercfg->lookup_user_data($username, $noerr);
> +	    return $data->{role} if $data && $data->{enable};
>  	}
>      }
>  
> @@ -123,6 +149,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/PAM.pm b/src/PMG/Auth/PAM.pm
> new file mode 100755
> index 0000000..3ffd8a6
> --- /dev/null
> +++ b/src/PMG/Auth/PAM.pm
> @@ -0,0 +1,22 @@
> +package PMG::Auth::PAM;
> +
> +use strict;
> +use warnings;
> +
> +use PMG::Auth::Plugin;
> +
> +use base qw(PMG::Auth::Plugin);
> +
> +sub type {
> +    return 'pam';
> +}
> +
> +sub options {
> +    return {
> +	default => { optional => 1 },
> +	comment => { optional => 1 },
> +	tfa => { optional => 1 },
> +    };
> +}
> +
> +1;
> diff --git a/src/PMG/Auth/PMG.pm b/src/PMG/Auth/PMG.pm
> new file mode 100755
> index 0000000..b083692
> --- /dev/null
> +++ b/src/PMG/Auth/PMG.pm
> @@ -0,0 +1,39 @@
> +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 {
> +	default => {
> +	    description => "Use this as default realm",
> +	    type => 'boolean',
> +	    optional => 1,
> +	},
> +	comment => {
> +	    description => "Description.",
> +	    type => 'string',
> +	    optional => 1,
> +	    maxLength => 4096,
> +	},
> +	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..8e8492b
> --- /dev/null
> +++ b/src/PMG/Auth/Plugin.pm
> @@ -0,0 +1,203 @@
> +package PMG::Auth::Plugin;
> +
> +use strict;
> +use warnings;
> +
> +use Digest::SHA;
> +use Encode;
> +
> +use PMG::Utils;
> +use PVE::INotify;
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::Schema::Auth;
> +use PVE::SectionConfig;
> +use PVE::Tools;
> +
> +use base qw(PVE::SectionConfig);
> +
> +my $realm_cfg_id = "realms.cfg";
> +my $lockfile = "/var/lock/pmg-realms.lck";
> +
> +sub realm_cfg_id {
> +    return $realm_cfg_id;
> +}
> +
> +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(
> +    $realm_cfg_id,
> +    "/etc/pmg/realms.cfg",
> +    \&read_realms_conf,
> +    \&write_realms_conf,
> +    undef,
> +    always_call_parser => 1,
> +);
> +
> +sub lock_realm_config {
> +    my ($code, $errmsg) = @_;
> +
> +    PVE::Tools::lock_file($lockfile, undef, $code);
> +    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 $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 realms
> +    $cfg->{ids}->{pmg}->{type} = 'pmg'; # force type
> +    $cfg->{ids}->{pmg}->{comment} = "Proxmox Mail Gateway authentication server"
> +	if !$cfg->{ids}->{pmg}->{comment};
> +    $cfg->{ids}->{pmg}->{default} = 1
> +	if !$cfg->{ids}->{pmg}->{default};
> +
> +    $cfg->{ids}->{pam}->{type} = 'pam'; # force type
> +    $cfg->{ids}->{pam}->{comment} = "Linux PAM standard authentication"
> +	if !$cfg->{ids}->{pam}->{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 realm 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 realm config *locked* context
> +sub on_add_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
> +# called during realm configuration update (before the updated realm 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 realm config *locked* context
> +sub on_update_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
> +# called during deletion of realms (before the new realm 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 realm 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 realm config gets written)
> +# die to abort addition/update in case the connection/bind fails
> +# NOTE: runs in a realm 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..672f152 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,
>  	},
> @@ -188,6 +188,7 @@ my $fixup_root_properties = sub {
>      $cfg->{'root@pam'}->{expire} = 0;
>      $cfg->{'root@pam'}->{comment} = 'Unix Superuser';
>      $cfg->{'root@pam'}->{role} = 'root';
> +    $cfg->{'root@pam'}->{realm} = 'pam';
>      delete $cfg->{'root@pam'}->{crypt_pass};
>  };
>  
> @@ -217,26 +218,29 @@ sub read_user_conf {
>                 (?<firstname>(?:[^:]*)) :
>                 (?<lastname>(?:[^:]*)) :
>                 (?<keys>(?:[^:]*)) :
> +               ((?<realm>(?:[^:]*)) :)?
>                 $/x
>  	    ) {
> +		my $default_realm = ($+{userid} eq 'root') ? 'pam' : 'pmg';
> +		my $realm = $+{realm} || $default_realm;
>  		my $d = {
>  		    username => $+{userid},
> -		    userid => $+{userid} . '@pmg',
> -		    realm => 'pmg',
> +		    userid => $+{userid} . '@' . $realm,
> +		    realm => $realm,
>  		    enable => $+{enable} || 0,
>  		    expire => $+{expire} || 0,
>  		    role => $+{role},
>  		};
>  		$d->{comment} = $comment if $comment;
>  		$comment = '';
> -		foreach my $k (qw(crypt_pass email firstname lastname keys)) {
> +		foreach my $k (qw(crypt_pass email firstname lastname keys realm)) {
>  		    $d->{$k} = $+{$k} if $+{$k};
>  		}
>  		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} ne 'root@pam';
>  		};
>  		if (my $err = $@) {
>  		    warn "$filename: $err";
> @@ -274,10 +278,11 @@ sub write_user_conf {
>  	$verify_entry->($d);
>  	$cfg->{$d->{userid}} = $d;
>  
> +	my $realm_regex = PMG::Utils::valid_pmg_realm_regex();
>  	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_regex})!;
>  	}
>  
>  	my $line;
> @@ -287,12 +292,15 @@ sub write_user_conf {
>  	    $d->{crypt_pass} = '',
>  	    $d->{expire} = '0',
>  	    $d->{role} = 'root';
> +	    $d->{realm} = 'pam';
>  	} else {
> -	    next if $userid !~ m/^(?<username>.+)\@pmg$/;
> +	    next if $userid !~ m/^(?<username>.+)\@(${realm_regex})$/;
>  	    $line = "$+{username}:";
>  	}
> +	# default to pmg
> +	$d->{realm} //= 'pmg';
>  
> -	for my $k (qw(enable expire crypt_pass role email firstname lastname keys)) {
> +	for my $k (qw(enable expire crypt_pass role email firstname lastname keys realm)) {
>  	    $line .= ($d->{$k} // '') . ':';
>  	}
>  	if (my $comment = $d->{comment}) {
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index 0b8945f..04a3a2e 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -49,12 +49,30 @@ postgres_admin_cmd
>  try_decode_utf8
>  );
>  
> -my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
> +my $user_regex = qr![^\s:/]+!;
> +
> +sub valid_pmg_realm_regex {
> +    my $cfg = PVE::INotify::read_file('realms.cfg');
> +    my $ids = $cfg->{ids};
> +    my $realms = ['pam', 'quarantine', sort keys $cfg->{ids}->%* ];
> +    return join('|', @$realms);
> +}
> +
> +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;
> +}
> +
> +PVE::JSONSchema::register_format('pmg-realm', \&is_valid_realm);
>  
>  PVE::JSONSchema::register_standard_option('realm', {
>      description => "Authentication domain ID",
>      type => 'string',
> -    enum => $valid_pmg_realms,
> +    format => 'pmg-realm',
>      maxLength => 32,
>  });
>  
> @@ -82,16 +100,15 @@ sub verify_username {
>  	die "user name '$username' is too short\n" if !$noerr;
>  	return undef;
>      }
> -    if ($len > 64) {
> -	die "user name '$username' is too long ($len > 64)\n" if !$noerr;
> +    if ($len > 128) {
> +	die "user name '$username' is too long ($len > 128)\n" if !$noerr;
>  	return undef;
>      }
>  
>      # 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 $realm_list = join('|', @$valid_pmg_realms);
> -    if ($username =~ m!^([^\s:/]+)\@(${realm_list})$!) {
> +    if ($username =~ m!^(${user_regex})\@([A-Za-z][A-Za-z0-9\.\-_]+)$!) {
>  	return wantarray ? ($username, $1, $2) : $username;
>      }
>  



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