From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pmg-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id C3F7E1FF176
for <inbox@lore.proxmox.com>; Fri, 21 Feb 2025 13:36:09 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 0B9A2C20;
Fri, 21 Feb 2025 13:36:08 +0100 (CET)
Date: Fri, 21 Feb 2025 13:35:32 +0100 (CET)
From: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= <f.gruenbichler@proxmox.com>
To: Markus Frank <m.frank@proxmox.com>, pmg-devel@lists.proxmox.com
Message-ID: <1818641200.10110.1740141332285@webmail.proxmox.com>
In-Reply-To: <20250218161905.17224-5-m.frank@proxmox.com>
References: <20250218161905.17224-1-m.frank@proxmox.com>
<20250218161905.17224-5-m.frank@proxmox.com>
MIME-Version: 1.0
X-Priority: 3
Importance: Normal
X-Mailer: Open-Xchange Mailer v7.10.6-Rev73
X-Originating-Client: open-xchange-appsuite
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.044 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [restenvironment.pm, proxmox.com, accesscontrol.pm, utils.pm,
api2.pm, userconfig.pm, pmg.pm, plugin.pm, pam.pm, acmeplugin.pm,
nodeconfig.pm]
Subject: Re: [pmg-devel] [PATCH pmg-api v5 4/10] config: add plugin system
for realms
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
<pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pmg-devel-bounces@lists.proxmox.com
Sender: "pmg-devel" <pmg-devel-bounces@lists.proxmox.com>
> Markus Frank <m.frank@proxmox.com> hat am 18.02.2025 17:18 CET geschrieben:
>
>
> 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.
>
> 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.
>
> 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>
> ---
> src/Makefile | 3 +
> src/PMG/AccessControl.pm | 31 ++++++
> src/PMG/Auth/PAM.pm | 22 ++++
> src/PMG/Auth/PMG.pm | 39 ++++++++
> src/PMG/Auth/Plugin.pm | 199 +++++++++++++++++++++++++++++++++++++
> src/PMG/RESTEnvironment.pm | 14 +++
> src/PMG/UserConfig.pm | 25 +++--
> src/PMG/Utils.pm | 29 ++++--
> 8 files changed, 346 insertions(+), 16 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/AccessControl.pm b/src/PMG/AccessControl.pm
> index e12d7cf..ced5f9a 100644
> --- a/src/PMG/AccessControl.pm
> +++ b/src/PMG/AccessControl.pm
> @@ -13,6 +13,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 +46,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 +62,8 @@ sub authenticate_user : prototype($$$) {
> return ($pmail . '@quarantine', undef);
> }
> die "ldap login failed\n";
> + } elsif ($realm =~ m!(${realm_regex})!) {
> + # nothing to do for self-defined realms
this is really dangerous and this whole code should be restructured like it is in PVE! i.e., the authentication should happen in the plugin as plugin method, and OIDC should die because we should never end up there!
> } else {
> die "no such realm '$realm'\n";
> }
> @@ -79,6 +90,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 +104,8 @@ sub set_user_password {
>
> } elsif ($realm eq 'pmg') {
> PMG::UserConfig->set_user_password($username, $password);
> + } elsif ($realm =~ m!(${realm_regex})!) {
> + # nothing to do for self-defined realms
same here
> } else {
> die "no such realm '$realm'\n";
> }
> @@ -106,6 +120,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 +130,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};
> }
this is a bit pre-existing, but.. this whole method is weird and should be refactored to
- first check if the user is enabled in the config
- then call the plugin to get its role
> }
>
> @@ -123,6 +142,18 @@ sub check_user_enabled {
> return undef;
> }
>
> +sub check_user_exist {
what is this for? it is defined here, and re-exported in PMG::RESTEnvironment, but not used anywhere?
> + 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..b336d0c
> --- /dev/null
> +++ b/src/PMG/Auth/Plugin.pm
> @@ -0,0 +1,199 @@
> +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 $domainconfigfile = "realms.cfg";
> +my $lockfile = "/var/lock/pmg-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,
> +);
> +
> +sub lock_domain_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 domains
> + $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 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);
see above
> +}
> +
> sub check_node_is_master {
> my ($self, $noerr);
>
> diff --git a/src/PMG/UserConfig.pm b/src/PMG/UserConfig.pm
> index b9a83a7..fe6d2c8 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});
this is broken if you add a user with '@' in the username part (which is allowed I think?)
> + 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,23 @@ 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;
>
> 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_regex})$/;
> + $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 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;
> }
>
> --
> 2.39.5
>
>
>
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel