Thomas Lamprecht [Thu, 23 Mar 2023 14:44:29 +0000 (15:44 +0100)]
bump version to 7.4-2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Thu, 23 Mar 2023 13:14:29 +0000 (14:14 +0100)]
fix #4609: allow valid DN in ldap/ad realm config
We previously added support for ',' in the DNS attribute through
allowing a quoted format, but the regex used was made too
restrictive.
In the new quoted attribute we'd only allow \w (alphanumeric and _)
and the restricted characters. This patch now changes that to allow
everything except the quotation mark " itself, which is again closer
to the original regex which did not care for quotation and allowed
everything aside from ','.
The unquoted attributes did not allow spaces anymore, but the RFC [0]
actually makes it clear that spaces are only forbidden at the
beginning and the end (same for #). So, fix the regex to accommodate
for that and allow space and # characters again if not at the end or
beginning.
0: https://www.ietf.org/rfc/rfc2253.txt
Fixes: 1aa2355 ("ldap: Allow quoted values for DN attribute values")
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Friedrich Weber <f.weber@proxmox.com>
[ T: make fixes a trailer and rework commit message ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Mon, 20 Mar 2023 16:16:15 +0000 (17:16 +0100)]
bump version to 7.4-1
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Christoph Heiss [Tue, 31 Jan 2023 12:50:42 +0000 (13:50 +0100)]
ldap: Allow quoted values for DN attribute values
This fixes #3748 by allowing reserved characters in `bind_dn` (and
other properties of the same format) if they are properly quoted and
adds some corresponding documentation regarding that.
This was tested by setting up a slapd server and creating a user with
the CN `Test, User` much like in the bug report, then using this user
as `bind_dn` in the sync options. I also tested some variants of that
CN, including just `TestUser`.)
One thing that still won't work is syncing of LDAP users with colons
or slashes in their CNs. In such cases, the message
> value 'Test, User@ldap' does not look like a valid user name
will pop up.
This is due to spaces and colons being explicitly disallowed in
usernames by PVE access-control's username schema. This probably
means that such names can never be allowed, which is being documented
too as separate pve-docs patch.
Note that while this is now a bit more strict for some cases too,
they should not matter in practice. For context; see RFC 2253 [0],
section 4. Interestingly, this document was obsoleted by RFC 4514 [1]
in 2006, which only mentions this in section 2.4 ("Converting an
AttributeValue from ASN.1 to a String") and appendix A ("Presentation
Issues").
But the first one seems to be the "authoritive" document on this
matter, at least looking at some other docs about LDAP DNs (RedHat,
Microsoft, ..).
[0] https://www.ietf.org/rfc/rfc2253.txt
[1] https://docs.ldap.com/specs/rfc4514.txt
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
[ T: added commit message ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Tue, 17 Jan 2023 11:46:53 +0000 (12:46 +0100)]
realm sync: refactor scope/remove-vanished into a standard option
so that we can reuse it easily
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Thomas Lamprecht [Mon, 6 Mar 2023 10:40:15 +0000 (11:40 +0100)]
bump version to 7.3-2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Mon, 6 Mar 2023 09:41:37 +0000 (10:41 +0100)]
userid format: clarify that this is the full name@realm in description
as it recently confused a user in the forum.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Wed, 15 Feb 2023 09:44:13 +0000 (10:44 +0100)]
fix #4518: improve ACL computation performance
by switching to a tree-based in-memory structure, like we do in PBS.
instead of parsing ACL entries into a hash using the full ACL path as key for
each entry, parse them into a tree-like nested hash. when evaluating ACLs,
iterating over all path prefixes starting at '/' is needed anyway, so this is a
more natural way to store and access the parsed configuration.
some performance data, timing `pveum user permissions $user > /dev/null` for
various amounts of ACL entries in user.cfg
entries | stock | patched | speedup
-------------------------------------
1k | 1.234s | 0.241s | 5.12
2k | 4.480s | 0.262s | 17.09
20k | 7m25s | 0.987s | 450.86
similarly, an /access/ticket request such as the one happening on login goes
down from 4.27s to 109ms with 2k entries (testing with 20k entries fails
because the request times out after 30s, but with the patch it takes 336ms).
the underlying issue is that these two code paths not only iterate over *all
defined ACL paths* to get a complete picture of a user's/token's privileges,
but the fact that that ACL computation for each checked path itself did another
such loop in PVE::AccessControl::roles().
it is enough to iterate over the to-be-checked ACL path in a component-wise
fashion in order to handle role propagation, e.g., when looking at /a/b/c/d,
iterate over
/
/a
/a/b
/a/b/c
/a/b/c/d
in turn instead of all defined ACL paths.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Tue, 10 Jan 2023 13:49:35 +0000 (14:49 +0100)]
d/control: wrap-and-sort
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Tue, 10 Jan 2023 13:48:50 +0000 (14:48 +0100)]
build: add missing build-dependency
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Fri, 16 Dec 2022 12:11:08 +0000 (13:11 +0100)]
bump version to 7.3-1
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Tue, 6 Dec 2022 11:06:30 +0000 (12:06 +0100)]
realm: sync: allow explicit 'none' for 'remove-vanished' option
with that, the api call can now override the default option
that is set on the realm (if any) by providing 'none'
it was not possible previously to override the realm default
when one wanted no properties to delete
no other code changes are necessary since we only extract the
known values 'acl' etc. and 'none' has no meaning there
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Thomas Lamprecht [Thu, 17 Nov 2022 12:09:21 +0000 (13:09 +0100)]
bump version to 7.2-5
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Wed, 28 Sep 2022 12:50:47 +0000 (14:50 +0200)]
privs: add Sys.Incoming
for guarding cross-cluster data streams like guest migrations and
storage migrations.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Dominik Csapak [Fri, 21 Oct 2022 08:31:17 +0000 (10:31 +0200)]
authenticate_user: pass undef instead of empty $tfa_challenge to authenticate_2nd_new
just above, we check & return if $tfa_challenge is set, so there is no
way that it would be set here. To make it clearer that it must be undef
here pass it as such.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Dominik Csapak [Fri, 21 Oct 2022 08:31:16 +0000 (10:31 +0200)]
authenticate_2nd_new: rename $otp to $tfa_response
since that is what it really is, not only a otp
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Dominik Csapak [Fri, 21 Oct 2022 08:31:15 +0000 (10:31 +0200)]
authenticate_2nd_new: only lock tfa config for recovery keys
we currently only need to lock the tfa config when we got a recovery key
as a tfa challenge response, since that's the only thing that can
actually change the tfa config (every other method only reads from
there).
so to do that, factor out the code that was inside the lock, and call it
with/without lock depending on the tfa challenge response
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Stoiko Ivanov [Mon, 29 Aug 2022 16:07:55 +0000 (18:07 +0200)]
auth ldap/ad: compare group member dn case-insensitively
currently we add a user to a group if it's DN is listed in the
member-attributes of a group. The comparison for this is done via
existence check of a hash key, which is case-sensitive.
The equality for DNs is defined in a not straight forward way [0]:
(roughly translating to you need to honor the equality rules for each
'component' (RDN) of the DN) and is implementation-specific (Microsoft
AD is case-insensitive).
While this patch does not address the complete complexity of comparing
DNs it should work fine in practice.
issue with case-sensitive mismatches was reported in our community
forum:
https://forum.proxmox.com/threads/.113387
tested against a local test-vm used for reproducing the issue.
[0] https://ldapwiki.com/wiki/Distinguished%20Name%20Case%20Sensitivity
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
Wolfgang Bumiller [Mon, 25 Jul 2022 11:52:56 +0000 (13:52 +0200)]
tfa: pass whole webauthn config to 'set_webauthn_config'
the field names are being kept compatible
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Tue, 19 Jul 2022 10:07:58 +0000 (12:07 +0200)]
api: realm sync: avoid separate log line for "remove-vanished" opt
can be a bit confusing and not really necessary, just inline it with
the "syncing user/group" message
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 14 Jul 2022 06:36:58 +0000 (08:36 +0200)]
bump version to 7.2-4
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Wed, 13 Jul 2022 13:13:59 +0000 (15:13 +0200)]
auth key: fix double rotation in clusters
there is a (hard to trigger) race that can cause a double rotation of
the auth key, with potentially confusing fallout (various processes on
different nodes having an inconsistent view of the current and previous
auth keys, resulting in "random" invalid ticket errors until the next
proper key rotation 24h later).
the underlying cause is that `stat()` calls are excempt from our
otherwise non-cached/direct_io handling of pmxcfs I/O, which allows the
following sequence of events to take place:
LAST: mtime of current auth key
- current epoch advances to LAST + 24h
the following can be arbitrarly interleaved between node A and B:
- LAST+24h node A: pvedaemon/pvestatd on node A calls check_authkey(1)
- LAST+24h node A: it returns 0 (rotation required)
- LAST+24h node A: rotate_key() is called
- LAST+24h node A: cfs_lock_authkey is called
- LAST+24h node B: pvedaemon/pvestatd calls check_authkey(1)
- LAST+24h node B: key is not yet cached in-memory by current process
- LAST+24h node B: key file is opened, stat-ed, read, parsed, and content+mtime
is cached (the kernel will now cache this stat result for 1s unless
the path is opened)
- LAST+24h node B: it returns 0 (rotation required)
- LAST+24h node B: rotate_key() is called
- LAST+24h node B: cfs_lock_authkey is called
the following is mutex-ed via a cfs_lock:
- LAST+24h node A: lock is obtained
- LAST+24h node A: check_authkey() is called
- LAST+24h node A: key is stat-ed, mtime is still (correctly) LAST,
cached mtime and content are returned
- LAST+24h node A: it returns 0 (rotation still required)
- LAST+24h node A: get_pubkey() is called and returns current auth key
- LAST+24h node A: new keypair is generated and persisted
- LAST+24h node A: cfs_lock is released
- LAST+24h node B: changes by node A are processed by pmxcfs
- LAST+24h node B: lock is obtained
- LAST+24h node B: check_authkey() is called
- LAST+24h node B: key is stat-ed, mtime is (incorrectly!) still LAST
since the stat call is handled by the kernel/page cache, not by
pmxcfs, cached mtime and content are returned
- LAST+24h node B: it returns 0 (rotation still required)
- LAST+24h node B: get_pubkey() is called and returns either previous or
key written by node A (depending on whether page cache or pmxcfs
answers stat call)
- LAST+24h node B: new keypair is generated, key returned by last
get_pubkey call is written as old key
the end result is that some nodes and process will treat the key
generated by node A as "current", while others will treat the one
generated by nodoe B as "current". both have the same mtime, so the
in-memory cache hash won't be updated unless the service is restarted or
another rotation happens. depending on who generated the ticket and who
attempts validating it, a ticket might be rejected as invalid even
though the generating party would treat it as valid, and time on all
nodes is properly synced.
there seems to be now way for pmxcfs to pro-actively invalidate the page
cache entry safely (since we'd need to do so while writes to the same
path can happen concurrently), so work around by forcing an open/close
at the (stat) call site which does the work for us. regular reads are
not affected since those already bypass the page cache entirely anyway.
thankfully in almost all cases, the following sequence has enough
synchronization overhead baked in to avoid triggering the issue almost
entirely:
- cfs_lock
- generate key
- create tmp file for old key
- write tmp file
- rename tmp file into proper place
- create tmp file for new pub key
- write tmp file
- rename tmp file into proper place
- create tmp file for new priv key
- write tmp file
- rename tmp file into proper place
- release lock
that being said, there has been at least one report where this was
triggered in the wild from time to time.
it is easy to reproduce by increasing the attr_timeout and entry_timeout
fuse settings inside pmxcfs to increase the time stat results are
treated as valid/retained in the page cache:
-----8<-----
diff --git a/data/src/pmxcfs.c b/data/src/pmxcfs.c
index
d78a248..
e3e807b 100644
--- a/data/src/pmxcfs.c
+++ b/data/src/pmxcfs.c
@@ -935,7 +935,7 @@ int main(int argc, char *argv[])
mkdir(CFSDIR, 0755);
- char *fa[] = { "-f", "-odefault_permissions", "-oallow_other", NULL};
+ char *fa[] = { "-f", "-odefault_permissions", "-oallow_other", "-oentry_timeout=5", "-oattr_timeout=5", NULL};
struct fuse_args fuse_args = FUSE_ARGS_INIT(sizeof (fa)/sizeof(gpointer) - 1, fa);
----->8-----
in which case it's even easy to trigger more than double rotation in a
bigger test cluster (stopping all PVE services except for pve-cluster
helps avoiding interference):
on a single node:
$ touch --date yesterday /etc/pve/authkey.pub
in parallel (i.e., via tmux synchronized panes):
-----8<-----
use strict;
use warnings;
use PVE::Cluster;
use PVE::AccessControl;
PVE::Cluster::cfs_update();
# ensure page cache entry is there
PVE::AccessControl::check_authkey(1);
PVE::AccessControl::check_authkey(1);
# now attempt rotation
PVE::AccessControl::rotate_authkey();
----->8-----
Thanks to Wolfgang Bumiller for assistance in triaging and exploring
various avenues of fixing.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Mira Limbeck [Wed, 15 Jun 2022 14:09:50 +0000 (16:09 +0200)]
fix #4074: increase API OpenID code size limit to 2048
Azure AD seems to have a variable authorization code size, depending on
the browser state according to one report in bug #4074 [0].
Sometimes the size is greater than our current limit of 1024, so
increase it to 2048.
The RFC [1] mentions that there is no limit to the code size, but based on
current experience, a size limit of 2048 might be enough for every
current OpenID Connect provider.
[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4074
[1] https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2
Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
[w.bumiller@proxmox.com: bump to 4096]
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Mon, 20 Jun 2022 13:51:19 +0000 (15:51 +0200)]
bump version to 7.2-3
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Mon, 20 Jun 2022 11:05:12 +0000 (13:05 +0200)]
perm check: forbid undefined/empty ACL path
to detect similar issues to that fixed in the previous commit early on
and without the potential for dangerous side-effects.
root@pam is intentionally still allowed before the check in case such
situations can be triggered by misconfiguration - root@pam can then
still clean up the affected configs via the GUI/API, and not just via
manual editing.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Mon, 20 Jun 2022 11:05:11 +0000 (13:05 +0200)]
api2: token: use userid-group as API perm check
the previous version using an ACL path of '/access/users/{userid}' was
broken for non-root users, since the '@' character always contained in a
userid is not allowed in ACL paths.
this effectively meant that creating API tokens only worked for:
- root@pam (ACL checks skipped altogether)
- users with User.Modify on '/' with propagation (the roles/privs for
'/' are propagated to the undefined path in this case)
- users creating their own tokens (first branch of 'or')
the userid-group check is used for all other modifications of user
entities, so it can also be used for creating/modifying/removing API
tokens.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Fri, 3 Jun 2022 12:02:35 +0000 (14:02 +0200)]
bump version to 7.2-2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 3 Jun 2022 11:58:46 +0000 (13:58 +0200)]
README: break long lines
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 3 Jun 2022 11:58:07 +0000 (13:58 +0200)]
tree wide: typo fixes
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Fri, 3 Jun 2022 11:50:49 +0000 (13:50 +0200)]
permissions: add some more comments
for future reference/changes.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Fri, 3 Jun 2022 11:50:48 +0000 (13:50 +0200)]
permissions: fix token/user priv intersection
the token/user priv intersection could only honored user privs that had
the propagation flag set, reducing the scope of the token more than
intended.
the pre-existing test case actually triggered the broken behaviour, but
the expected value matched it so it was not noticed.
Fixes: e8a0cee47bb477162f291e67144ea0c0df47f2ee "rpcenv: improve user/token intersection"
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Fri, 3 Jun 2022 11:50:47 +0000 (13:50 +0200)]
permissions: properly merge propagation flag
when multiple roles are defined on a path that share a privilege, this
randomly took the propagation flag for the priv from the last role
encountered. since perl hashes are iterated randomly, this means the
propagation flag was sometimes set correctly, and sometimes not.
note that this propagation flag is only used for display/dumping
purposes, and for intersection with token privs (see next commit).
actual handling of propagation happens on the role level in
PVE::AccessControl::roles().
modified test case (spuriously) fails without the fix.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Tue, 31 May 2022 11:43:47 +0000 (13:43 +0200)]
bump version to 7.2-1
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Tue, 31 May 2022 11:32:36 +0000 (13:32 +0200)]
access check: include user/token id in expired exception
not that relevant for the user as the daemon auth log already
contains that info, but for token it can be nice.
The API response is always just a plain "401 auth failure" in any
case (expired or wrong creds)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Tue, 31 May 2022 11:31:23 +0000 (13:31 +0200)]
check user: re-order enable check first again and adhere noerr flag
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Oguz Bektas [Tue, 31 May 2022 10:58:52 +0000 (12:58 +0200)]
user check: fix expiration/enable order
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 28 Apr 2022 15:02:54 +0000 (17:02 +0200)]
bump version to 7.1-8
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Mon, 28 Mar 2022 12:38:05 +0000 (14:38 +0200)]
add realm-sync regression test for new 'remove-vanished'
by having a test case that does not delete properties, but acls and
entries
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Mon, 28 Mar 2022 12:38:04 +0000 (14:38 +0200)]
convert regression tests to new 'remove-vanished' parameter
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Mon, 28 Mar 2022 12:38:03 +0000 (14:38 +0200)]
fix #3668: realm-sync: replace 'full' & 'purge' with 'remove-vanished'
Both, 'full' & 'purge', configure what is removed when an entry
(or property) is not returned anymore from the sync result. For users
it may be thus easier to understand setting a single property without
wondering about the interaction of the two specific ones.
The new 'remove-vanished' is a list of things to remove, namely:
* 'acl'
* 'entry'
* 'properties'.
The old 'full' gets mapped to 'entry;properties' and an old 'purge'
to the 'acl'
Keep the old properties for backwards-compatibility but transform to
the new list on edit. Add a deprecation notice to the description
with a TODO reminder to remove with 8.0
Note, this commit changes two things slightly:
* 'purge' (now remove-vanished -> acl) does now remove ACLs for
vanished entries even when we keep those entries. Previously those
were only deleted when we also removed the entries (this can be
seen by the changed regression test)
* since 'remove-vanished' is empty by default, only the scope must be
given explicitly on sync or the sync-default. Previously 'full' and
'purge' must have been configured explicitly (no big deal, since
the default is to not remove anything)
Bug #3668 is fixed when not selecting 'properties' for the sync, so
that existing (custom) properties are not deleted on update
Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
[ T: some commit message typos/wording ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Mon, 28 Mar 2022 12:38:02 +0000 (14:38 +0200)]
add regression tests for realm-sync
to fully test the 'end-to-end' sync api call, we have to mock quite
some methods for cluster/rpcenvironment/ldap
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 23 Mar 2022 11:24:57 +0000 (12:24 +0100)]
rpc env: indentation fixes
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Mon, 21 Mar 2022 15:15:29 +0000 (16:15 +0100)]
bump version to 7.1-7
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Mon, 21 Mar 2022 14:29:12 +0000 (15:29 +0100)]
api: get user: declare token schema
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Fabian Grünbichler [Thu, 17 Mar 2022 08:57:21 +0000 (09:57 +0100)]
userid-group check: distinguish create and update
and check both existing groups and the groups parameter in the update
case. the following user.cfg settings can be used for testing:
user:test@pve:1:0:t::::x:
user:other@pve:1:0:t::::x:
group:test:test@pve::
group:test3:::
role:RealmUserAllocator:Realm.AllocateUser:
role:UserModifier:User.Modify:
acl:0:/access/groups/test:test@pve:UserModifier:
acl:0:/access/groups/test3:@test:UserModifier:
acl:0:/access/realm/pve:test@pve:RealmUserAllocator:
unchanged: the user 'test@pve' can allocate new '@pve' users, but
only if the created user will belong to at least one of 'test'
(direct ACL for that user) or 'test3' (indirect ACL via 'test' group)
groups.
changed: if the user 'test@pve' updates an existing user, they need
to (A) have 'User.Modify' on at least one existing group of that
user, and (B) 'User.Modify' on all of the groups passed in via the
'groups' parameter. A is the general rule for 'allowed to modify
user' across the board, but was missing for this specific variant of
the check. B was the case before, but just checking this without also
checking A allows a user to pull off-limits users into groups that
they can modify, which then in turn allows them to modify those users
via A which is now passing.
for example, without this patch 'test@pve' would be able to add
'other@pve' to either 'test' or 'test3', and then in turn call any of
the API endpoints that require 'User.Modify' on a user's group
(change TFA, change password or delete user if realm is pve, ..).
all the other userid-group checks without group_param set remain
unchanged as well, since $check_existing_user is true in that case.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Fri, 21 Jan 2022 13:20:58 +0000 (14:20 +0100)]
bump version to 7.1-6
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Fri, 21 Jan 2022 10:57:30 +0000 (11:57 +0100)]
rpcenv->permissions() ensure propagate is always defined
this shouldn't happen anymore, but a safeguard in case the parser ever
has a bug does not hurt.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Fri, 21 Jan 2022 10:52:08 +0000 (11:52 +0100)]
rpcenv: skip undef propagation flag in more places
these are just cosmetic fixes/safeguards against future bugs -
compute_api_permissions is used to set the 'cap' object to hide parts of
the GUI that are not usable without the corresponding privs in the
backend anyway, and get_effective_permissions is only used to return the
permission tree without a specific path query.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Fri, 21 Jan 2022 10:23:25 +0000 (11:23 +0100)]
test: extend intersection tests
to check the previous commit's fix.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Fri, 21 Jan 2022 10:23:36 +0000 (11:23 +0100)]
rpcenv: improve user/token intersection
this could return undef for the propagation flag instead of 1/0, leading
to confusing displays of permission trees. all the actual checks using
the returned hash check for definedness anyway, so the actual
privileges checked and the displayed ones were not identical.
fixes:
7e8bcaa754432f084e53d9daf13c653a8777d88b
"roles()/permissions(): also return propagate flag"
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Fri, 21 Jan 2022 13:19:55 +0000 (14:19 +0100)]
get effective permissions: return /sdn path too
only cosmetic for the user to see what their permission tree looks
like.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Mon, 20 Dec 2021 10:31:15 +0000 (11:31 +0100)]
fix realm sync permissions
The userid-* permission check variants work on
$param->{userid} directly which does not exist for this
call. Also, they work on the realm of the user being
checked, rather than the realm provided as parameter.
The result was that as non-root user this always failed
with the message "userid '' too short"
Fix this by making the check explicitly work like in the
description.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Tue, 14 Dec 2021 08:13:44 +0000 (09:13 +0100)]
use statement fixup
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 6 Dec 2021 13:33:44 +0000 (14:33 +0100)]
tfa list: account for admin permissions
instead of restricting listing tfa entries of others to
root@pam, perform the same checks the user-list does and
which also reflect the permissions of the api calls actually
operating on those users, so, `User.Modify` on the user (but
also `Sys.Audit`, since it's only a read-operation, just
like the user index API call)
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 6 Dec 2021 13:12:33 +0000 (14:12 +0100)]
tfa: when modifying others, verify the current user's password
this was wrong as it asked for the password of the
to-be-edited user instead, which makes no sense
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 6 Dec 2021 08:38:01 +0000 (09:38 +0100)]
fix #3768: warn on bad u2f or webauthn settings
but don't bail out of the entire auth process, otherwise
not even totp or recovery keys will work anymore in this
case
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 6 Dec 2021 08:38:00 +0000 (09:38 +0100)]
fix a 'use of undefined...' warning
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Thu, 25 Nov 2021 06:57:45 +0000 (07:57 +0100)]
bump version to 7.1-5
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 25 Nov 2021 06:57:10 +0000 (07:57 +0100)]
openid: fix username-claim fallback
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Mon, 22 Nov 2021 13:04:57 +0000 (14:04 +0100)]
bump version to 7.1-4
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 22 Nov 2021 12:52:57 +0000 (13:52 +0100)]
fill origin into webauthn config if not provided
in order to allow subdomains to work, the wa config should
only specify 'id' and 'rp', the 'origin' gets filled in by
the node
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Fri, 19 Nov 2021 07:12:02 +0000 (08:12 +0100)]
bump version to 7.1-3
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 19 Nov 2021 08:34:25 +0000 (09:34 +0100)]
d/control: bump versioned dependency to libpve-rs-perl
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 18 Nov 2021 16:01:04 +0000 (17:01 +0100)]
openid: support configuring ACR values
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 18 Nov 2021 16:00:42 +0000 (17:00 +0100)]
openid: support configuring scopes
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 18 Nov 2021 13:53:44 +0000 (14:53 +0100)]
openid: support configuring the prompt
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 18 Nov 2021 13:52:52 +0000 (14:52 +0100)]
openid: fix whitespace/indentation
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 18 Nov 2021 13:24:24 +0000 (14:24 +0100)]
openid: allow arbitrary username-claims
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 17 Nov 2021 12:45:08 +0000 (13:45 +0100)]
bump version to 7.1-2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 17 Nov 2021 12:47:59 +0000 (13:47 +0100)]
d/control: bump versioned dependency to libpve-rs-perl
to ensure we get the incompatible type set for such TFA entries
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Wed, 17 Nov 2021 11:34:40 +0000 (12:34 +0100)]
catch incompatible tfa entries with a nice error
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Mon, 15 Nov 2021 14:33:29 +0000 (15:33 +0100)]
bump version to 7.1-1
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Fri, 12 Nov 2021 09:37:45 +0000 (10:37 +0100)]
tfa: fix http 404 in get_tfa_entry
this produced warnings in the journal and returned code 500
instead
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Thu, 11 Nov 2021 17:20:02 +0000 (18:20 +0100)]
bump version to 7.0-7
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 11 Nov 2021 17:19:23 +0000 (18:19 +0100)]
d/control: break pve-manager (<< 7.0-15)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Thu, 11 Nov 2021 14:07:12 +0000 (15:07 +0100)]
ticket: normalize path for verification
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Thu, 11 Nov 2021 14:07:11 +0000 (15:07 +0100)]
tickets: add tunnel ticket
just like VNC ticket, but different prefix to prevent confusion.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Thu, 11 Nov 2021 11:29:58 +0000 (12:29 +0100)]
tfa: upgrade check: early return if no cluster members
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 11 Nov 2021 11:06:42 +0000 (12:06 +0100)]
tfa: upgrade check: more info in error message(s)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 11 Nov 2021 11:05:52 +0000 (12:05 +0100)]
tfa: upgrade check: be less strict about version format
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Wed, 10 Nov 2021 14:11:45 +0000 (15:11 +0100)]
implement version checks for tfa
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Wed, 10 Nov 2021 12:49:03 +0000 (13:49 +0100)]
merge old user.cfg keys to tfa config when adding entries
this happens when the first new tfa entry is added and the
'keys' entry is replaced by "x"
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Wed, 10 Nov 2021 12:49:02 +0000 (13:49 +0100)]
d/control: add liburi-perl dependency
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Wed, 10 Nov 2021 12:49:01 +0000 (13:49 +0100)]
check enforced realm tfa type in new auth
this way we could also add webauthn as a required tfa type
to the realm configs later on
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Wed, 10 Nov 2021 12:49:00 +0000 (13:49 +0100)]
assert tfa/user config lock order
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:27:05 +0000 (12:27 +0100)]
set/remove 'x' for tfa keys in user.cfg in new api
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:27:04 +0000 (12:27 +0100)]
pveum: update tfa delete command
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:27:03 +0000 (12:27 +0100)]
update tfa cleanup when deleting users
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:27:02 +0000 (12:27 +0100)]
support registering yubico otp keys
In PBS we don't support this, so the current TFA API in rust
does not support this either (although the config does know
about its *existence*).
For now, yubico authentication will be done in perl. Adding
it to rust the rust TFA crate would not make much sense
anyway as we'd likely not want to use the same http client
crate in pve and pbs anyway (since pve is all blocking code
and pbs is async...)
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:27:01 +0000 (12:27 +0100)]
add pbs-style TFA API implementation
implements the same api paths as in pbs by forwarding the
api methods to the rust implementation after performing the
product-specific checks
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:27:00 +0000 (12:27 +0100)]
move TFA api path into its own module
and remove old modification api
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:26:59 +0000 (12:26 +0100)]
handle yubico authentication in new path
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:26:58 +0000 (12:26 +0100)]
use PBS-like auth api call flow
The main difference here is that we have no separate api
path for verification. Instead, the ticket api call gets an
optional 'tfa-challenge' parameter.
This is opt-in: old pve-manager UI with new
pve-access-control will still work as expected, but users
won't be able to *update* their TFA settings.
Since the new tfa config parser will build a compatible
in-perl tfa config object as well, the old authentication
code is left unchanged for compatibility and will be removed
with pve-8, where the `new-format` parameter in the ticket
call will change its default to `1`.
The `change_tfa` call will simply die in this commit. It is
removed later when adding the pbs-style TFA API calls.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:26:57 +0000 (12:26 +0100)]
update read_user_tfa_type call
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 9 Nov 2021 11:26:56 +0000 (12:26 +0100)]
use rust parser for TFA config
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Wed, 3 Nov 2021 10:30:05 +0000 (11:30 +0100)]
openid: proxy: only set env var if DC-config property exists
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Tue, 13 Jul 2021 08:09:17 +0000 (10:09 +0200)]
fix #3513: pass configured proxy to OpenID
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Thu, 21 Oct 2021 10:28:58 +0000 (12:28 +0200)]
bump version to 7.0-6
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Tue, 19 Oct 2021 11:52:42 +0000 (13:52 +0200)]
fix user deletion when realm does not enforce TFA
here the existance of the user is only interesting if we want to set
data, not if we delete it.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Thomas Lamprecht [Mon, 27 Sep 2021 13:50:50 +0000 (15:50 +0200)]
bump version to 7.0-5
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>