From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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 <pmg-devel@lists.proxmox.com>; 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 <pmg-devel@lists.proxmox.com>; 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 <pmg-devel@lists.proxmox.com>; 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 <pmg-devel@lists.proxmox.com>; 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 <m.frank@proxmox.com>, 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 <t.lamprecht@proxmox.com>
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
<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>
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 <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
>=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 {
> (?<keys>(?:[^:]*)) :
> $/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/^(?<username>.+)\@pmg$/;
> - $line =3D "$+{username}:";
> + next if $userid !~ m/^(?<username>.+)\@(${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