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.
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.
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.
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>
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>
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.
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.
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>
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>
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>
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>
projects / pve-access-control.git / log