From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6F83F918CA for ; Thu, 4 Apr 2024 13:58:15 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4BCCF142 for ; Thu, 4 Apr 2024 13:57:45 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 4 Apr 2024 13:57:43 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2452A452F6 for ; Thu, 4 Apr 2024 13:57:43 +0200 (CEST) Message-ID: <1c49e373-ea30-4acf-a074-7dd57e64cf13@proxmox.com> Date: Thu, 4 Apr 2024 13:57:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Markus Frank , pmg-devel@lists.proxmox.com References: <20240402112721.14405-1-m.frank@proxmox.com> <20240402112721.14405-3-m.frank@proxmox.com> Content-Language: en-GB, de-AT From: Thomas Lamprecht Autocrypt: addr=t.lamprecht@proxmox.com; keydata= xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD 90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7 UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+ wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6 YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/ bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1 feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95 QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6 NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM 8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9 GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j /Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0 F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg 8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja 0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx In-Reply-To: <20240402112721.14405-3-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.058 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pmg-devel] [PATCH pmg-api 2/6] config: add plugin system for realms & add openid type realms X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Apr 2024 11:58:15 -0000 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. >=20 > 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 chan= ged and all that, having some background and reference can only help >=20 > Signed-off-by: Markus Frank > --- > 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 >=20 > diff --git a/src/Makefile b/src/Makefile > index 8e49a10..407cc4a 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -164,6 +164,9 @@ LIBSOURCES =3D \ > 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 touc= h existing lines) > =20 > SOURCES =3D ${LIBSOURCES} ${CLI_BINARIES} ${TEMPLATES_FILES} ${CONF_MA= NS} ${CLI_MANS} ${SERVICE_MANS} ${SERVICE_UNITS} ${TIMER_UNITS} pmg-sourc= es.list pmg-initramfs.conf > =20 > 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.=20 > =20 > use PMG::UserConfig; > use PMG::LDAPConfig; > use PMG::LDAPSet; > use PMG::TFAConfig; > =20 > +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 =3D shift; > =20 > @@ -38,6 +47,8 @@ sub authenticate_user : prototype($$$) { > =20 > ($username, $ruid, $realm) =3D PMG::Utils::verify_username($userna= me); > =20 > + my $valid_pmg_realms =3D PMG::Utils::valid_pmg_realms(); > + my $realm_list =3D 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 =3D~ 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 { > =20 > ($username, $ruid, $realm) =3D PMG::Utils::verify_username($userna= me); > =20 > + my $valid_pmg_realms =3D PMG::Utils::valid_pmg_realms(); > + my $realm_list =3D join('|', @$valid_pmg_realms); > if ($realm eq 'pam') { > die "invalid pam user (only root allowed)\n" if $ruid ne 'root'; > =20 > @@ -92,6 +106,7 @@ sub set_user_password { > =20 > } elsif ($realm eq 'pmg') { > PMG::UserConfig->set_user_password($username, $password); > + } elsif ($realm =3D~ m!(${realm_list})!) { same here > } else { > die "no such realm '$realm'\n"; > } > @@ -106,6 +121,8 @@ sub check_user_enabled { > =20 > ($username, $ruid, $realm) =3D PMG::Utils::verify_username($userna= me, 1); > =20 > + my $valid_pmg_realms =3D PMG::Utils::valid_pmg_realms(); > + my $realm_list =3D 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 =3D~ m!(${realm_list})!) { > + my $usercfg =3D PMG::UserConfig->new(); > + my $data =3D $usercfg->lookup_user_data($username, $noerr); > + return $data->{role} if $data && $data->{enable}; > } > } > =20 > @@ -123,6 +144,18 @@ sub check_user_enabled { > return undef; > } > =20 > +sub check_user_exist { > + my ($usercfg, $username, $noerr) =3D @_; > + > + $username =3D 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) =3D @_; > =20 > 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, f= or 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")= =2E > +} > + > +sub properties { > + return { > + "issuer-url" =3D> { > + description =3D> "OpenID Issuer Url", > + type =3D> 'string', > + maxLength =3D> 256, > + }, > + "client-id" =3D> { > + description =3D> "OpenID Client ID", > + type =3D> 'string', > + maxLength =3D> 256, > + }, > + "client-key" =3D> { > + description =3D> "OpenID Client Key", > + type =3D> 'string', > + optional =3D> 1, > + maxLength =3D> 256, > + }, > + autocreate =3D> { > + description =3D> "Automatically create users if they do not exist= =2E", > + optional =3D> 1, > + type =3D> 'boolean', > + default =3D> 0, > + }, > + "username-claim" =3D> { > + description =3D> "OpenID claim used to generate the unique userna= me.", > + type =3D> 'string', > + optional =3D> 1, > + }, > + prompt =3D> { > + description =3D> "Specifies whether the Authorization Server prom= pts the End-User for" > + ." reauthentication and consent.", > + type =3D> 'string', > + pattern =3D> '(?:none|login|consent|select_account|\S+)', # \S+ i= s the extension variant > + optional =3D> 1, > + }, > + scopes =3D> { > + description =3D> "Specifies the scopes (user details) that should= be authorized and" > + ." returned, for example 'email' or 'profile'.", > + type =3D> 'string', # format =3D> 'some-safe-id-list', # FIXME: T= ODO > + default =3D> "email profile", > + optional =3D> 1, > + }, > + 'acr-values' =3D> { > + description =3D> "Specifies the Authentication Context Class Refe= rence values that the" > + ."Authorization Server is being requested to use for the Auth Reques= t.", > + type =3D> 'string', # format =3D> 'some-safe-id-list', # FIXME: T= ODO > + optional =3D> 1, > + }, > + default =3D> { > + description =3D> "Use this as default realm", > + type =3D> 'boolean', > + optional =3D> 1, > + }, > + comment =3D> { > + description =3D> "Description.", > + type =3D> 'string', > + optional =3D> 1, > + maxLength =3D> 4096, > + }, > + }; > +} > + > +sub options { > + return { small nit: here you use double-quotes for keys with a minus, but in the p= roperty schema definition you use single-quotes for them =E2=80=93 might also use= them here. > + "issuer-url" =3D> {}, > + "client-id" =3D> {}, > + "client-key" =3D> { optional =3D> 1 }, > + autocreate =3D> { optional =3D> 1 }, > + "username-claim" =3D> { optional =3D> 1, fixed =3D> 1 }, > + prompt =3D> { optional =3D> 1 }, > + scopes =3D> { optional =3D> 1 }, > + "acr-values" =3D> { optional =3D> 1 }, > + default =3D> { optional =3D> 1 }, > + comment =3D> { optional =3D> 1 }, > + }; > +} > + > +sub authenticate_user { > + my ($class, $config, $realm, $username, $password) =3D @_; > + > + 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 =3D> PVE::JSONSchema::get_standard_option('tfa'), > + }; > +} > + > +sub options { > + return { > + default =3D> { optional =3D> 1 }, > + comment =3D> { optional =3D> 1 }, > + tfa =3D> { optional =3D> 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 =3D "realms.cfg"; > +my $lockfile =3D "/var/lock/realms.lck"; > + > +sub read_realms_conf { > + my ($filename, $fh) =3D @_; > + > + my $raw; > + $raw =3D do { local $/ =3D undef; <$fh> } if defined($fh); > + > + return PMG::Auth::Plugin->parse_config($filename, $raw); > +} > + > +sub write_realms_conf { > + my ($filename, $fh, $cfg) =3D @_; > + > + my $raw =3D 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 =3D> 1 nit, missing trailing comma > +); > + > +sub lock_domain_config { > + my ($code, $errmsg) =3D @_; > + > + PVE::Tools::lock_file($lockfile, undef, $code); > + my $err =3D $@; > + if ($err) { nit: could avoid an extra line: if (my $err =3D $@) { ... > + $errmsg ? die "$errmsg: $err" : die $err; > + } > +} > + > +my $realm_regex =3D qr/[A-Za-z][A-Za-z0-9\.\-_]+/; > + > +sub pmg_verify_realm { > + my ($realm, $noerr) =3D @_; > + > + if ($realm !~ m/^${realm_regex}$/) { > + return undef if $noerr; > + die "value does not look like a valid realm\n"; > + } > + return $realm; > +} > + > +my $remove_options =3D "(?:acl|properties|entry)"; > + > +PVE::JSONSchema::register_standard_option('sync-scope', { > + description =3D> "Select what to sync.", > + type =3D> 'string', > + enum =3D> [qw(users groups both)], > + optional =3D> '1', > +}); > + > +PVE::JSONSchema::register_standard_option('sync-remove-vanished', { > + description =3D> "A semicolon-seperated list of things to remove w= hen 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 t= he 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 sy= nc." > + ." Instead of a list it also can be 'none' (the default).", > + type =3D> 'string', > + default =3D> 'none', > + typetext =3D> "([acl];[properties];[entry])|none", > + pattern =3D> "(?:(?:$remove_options\;)*$remove_options)|none", > + optional =3D> '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 re= use 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 =3D { > + scope =3D> get_standard_option('sync-scope'), > + 'remove-vanished' =3D> get_standard_option('sync-remove-vanished')= , > + 'enable-new' =3D> { > + description =3D> "Enable newly synced users immediately.", > + type =3D> 'boolean', > + default =3D> '1', > + optional =3D> '1', > + }, > +}; > +PVE::JSONSchema::register_standard_option('realm-sync-options', $realm= _sync_options_desc); > +PVE::JSONSchema::register_format('realm-sync-options', $realm_sync_opt= ions_desc); > + > +my $tfa_format =3D { > + type =3D> { > + description =3D> "The type of 2nd factor authentication.", > + format_description =3D> 'TFATYPE', > + type =3D> 'string', > + enum =3D> [qw(oath)], > + }, > + digits =3D> { > + description =3D> "TOTP digits.", > + format_description =3D> 'COUNT', > + type =3D> 'integer', > + minimum =3D> 6, maximum =3D> 8, > + default =3D> 6, > + optional =3D> 1, > + }, > + step =3D> { > + description =3D> "TOTP time period.", > + format_description =3D> 'SECONDS', > + type =3D> 'integer', > + minimum =3D> 10, > + default =3D> 30, > + optional =3D> 1, > + }, > +}; > + > +PVE::JSONSchema::register_format('pmg-tfa-config', $tfa_format); > + > +PVE::JSONSchema::register_standard_option('tfa', { > + description =3D> "Use Two-factor authentication.", > + type =3D> 'string', format =3D> 'pmg-tfa-config', > + optional =3D> 1, > + maxLength =3D> 128, > +}); > + > +sub parse_tfa_config { > + my ($data) =3D @_; > + > + return PVE::JSONSchema::parse_property_string($tfa_format, $data);= > +} > + > +my $defaultData =3D { > + propertyList =3D> { > + type =3D> { description =3D> "Realm type." }, > + realm =3D> get_standard_option('realm'), > + }, > +}; > + > +sub private { > + return $defaultData; > +} > + > +sub parse_section_header { > + my ($class, $line) =3D @_; > + > + if ($line =3D~ m/^(\S+):\s*(\S+)\s*$/) { > + my ($type, $realm) =3D (lc($1), $2); > + my $errmsg =3D undef; # set if you want to skip whole section > + eval { pmg_verify_realm($realm); }; > + $errmsg =3D $@ if $@; > + my $config =3D {}; # to return additional attributes > + return ($type, $realm, $errmsg, $config); > + } > + return undef; > +} > + > +sub parse_config { > + my ($class, $filename, $raw) =3D @_; > + > + my $cfg =3D $class->SUPER::parse_config($filename, $raw); > + > + my $default; > + foreach my $realm (keys %{$cfg->{ids}}) { > + my $data =3D $cfg->{ids}->{$realm}; > + # make sure there is only one default marker > + if ($data->{default}) { > + if ($default) { > + delete $data->{default}; > + } else { > + $default =3D $realm; > + } > + } > + > + if ($data->{comment}) { > + $data->{comment} =3D PVE::Tools::decode_text($data->{comment}); > + } > + > + } > + > + # add default domains > + $cfg->{ids}->{pmg}->{type} =3D 'pmg'; # force type > + $cfg->{ids}->{pmg}->{comment} =3D "Proxmox Mail Gateway authentica= tion server" > + if !$cfg->{ids}->{pmg}->{comment}; > + > + return $cfg; > +}; > + > +sub write_config { > + my ($class, $filename, $cfg) =3D @_; > + > + foreach my $realm (keys %{$cfg->{ids}}) { > + my $data =3D $cfg->{ids}->{$realm}; > + if ($data->{comment}) { > + $data->{comment} =3D PVE::Tools::encode_text($data->{comment}); > + } > + } > + > + $class->SUPER::write_config($filename, $cfg); > +} > + > +sub authenticate_user { > + my ($class, $config, $realm, $username, $password) =3D @_; > + > + die "overwrite me"; > +} > + > +sub store_password { > + my ($class, $config, $realm, $username, $password) =3D @_; > + > + my $type =3D $class->type(); > + > + die "can't set password on auth type '$type'\n"; > +} > + > +sub delete_user { > + my ($class, $config, $realm, $username) =3D @_; > + > + # do nothing by default > +} > + > +# called during addition of realm (before the new domain config got wr= itten) > +# `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) =3D @_; > + # 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 t= he 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) =3D @_; > + # do nothing by default > +} > + > +# called during deletion of realms (before the new domain config got w= ritten) > +# 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) =3D @_; > + # 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) =3D @_; > + # 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}; > } > =20 > +sub check_user_enabled { > + my ($self, $user, $noerr) =3D @_; > + > + my $cfg =3D $self->{usercfg}; > + return PMG::AccessControl::check_user_enabled($cfg, $user, $noerr)= ; > +} > + > +sub check_user_exist { > + my ($self, $user, $noerr) =3D @_; > + > + my $cfg =3D $self->{usercfg}; > + return PMG::AccessControl::check_user_exist($cfg, $user, $noerr); > +} > + > sub check_node_is_master { > my ($self, $noerr); > =20 > 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 =3D { > realm =3D> { > description =3D> "Authentication realm.", > type =3D> 'string', > - enum =3D> ['pam', 'pmg'], > + format =3D> 'pmg-realm', > default =3D> 'pmg', > optional =3D> 1, > }, > @@ -219,10 +219,13 @@ sub read_user_conf { > (?(?:[^:]*)) : > $/x > ) { > + my @username_parts =3D split('@', $+{userid}); Yuck on the pre-existing `$+{userid}` for , but oh well.. https://perldoc.perl.org/variables/$+ > + my $username =3D $username_parts[0]; > + my $realm =3D defined($username_parts[1]) ? $username_parts[1] : "pm= g"; > my $d =3D { > - username =3D> $+{userid}, > - userid =3D> $+{userid} . '@pmg', > - realm =3D> 'pmg', > + username =3D> $username, > + userid =3D> $username . '@' . $realm, > + realm =3D> $realm, > enable =3D> $+{enable} || 0, > expire =3D> $+{expire} || 0, > role =3D> $+{role}, > @@ -235,8 +238,9 @@ sub read_user_conf { > eval { > $verify_entry->($d); > $cfg->{$d->{userid}} =3D $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 =3D $@) { > warn "$filename: $err"; > @@ -274,22 +278,24 @@ sub write_user_conf { > $verify_entry->($d); > $cfg->{$d->{userid}} =3D $d; > =20 > + my $valid_pmg_realms =3D PMG::Utils::valid_pmg_realms(); > + my $realm_list =3D 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})!; > } > =20 > my $line; > =20 > if ($userid eq 'root@pam') { > - $line =3D 'root:'; > + $line =3D 'root@pam:'; > $d->{crypt_pass} =3D '', > $d->{expire} =3D '0', > $d->{role} =3D 'root'; > } else { > - next if $userid !~ m/^(?.+)\@pmg$/; > - $line =3D "$+{username}:"; > + next if $userid !~ m/^(?.+)\@(${realm_list})$/; > + $line =3D "$d->{userid}:"; > } > =20 > 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 > ); > =20 > -my $valid_pmg_realms =3D ['pam', 'pmg', 'quarantine']; > +my $user_regex =3D qr![^\s:/]+!; > + > +sub valid_pmg_realms { > + my $cfg =3D PVE::INotify::read_file('realms.cfg'); > + my $ids =3D $cfg->{ids}; > + my $realms =3D []; > + my @list =3D keys %{$ids}; > + for my $realm (@list) { > + push @$realms, $realm; > + } This can be quite a bit reduced to: my $realms =3D [ sort keys $cfg->{ids}->%* ]; > + push @$realms, 'pam'; > + push @$realms, 'quarantine'; can be fine like this, but FWIW, you could push multiple values at once o= r 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 va= lue as param and then implement that like: sub is_valid_realm { my ($realm) =3D @_; return 0 if !$realm; return 1 if $realm eq 'pam' || $realm eq 'quarantine'; # built-in o= nes my $cfg =3D 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); > =20 > PVE::JSONSchema::register_standard_option('realm', { > description =3D> "Authentication domain ID", > type =3D> 'string', > - enum =3D> $valid_pmg_realms, > + format =3D> 'pmg-realm', > maxLength =3D> 32, > }); > =20 > @@ -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 =3D valid_pmg_realms(); does this need the realm list here, i.e., isn't the existence of the real= m checked already elsewhere already so that we can use some general [a-z]+ for that= part in the regex here? > my $realm_list =3D join('|', @$valid_pmg_realms); > - if ($username =3D~ m!^([^\s:/]+)\@(${realm_list})$!) { > + if ($username =3D~ m!^(${user_regex})\@(${realm_list})$!) { > return wantarray ? ($username, $1, $2) : $username; > } > =20