IF YOU WOULD LIKE TO GET AN ACCOUNT, please write an
email to Administrator. User accounts are meant only to access repo
and report issues and/or generate pull requests.
This is a purpose-specific Git hosting for
BaseALT
projects. Thank you for your understanding!
Только зарегистрированные пользователи имеют доступ к сервису!
Для получения аккаунта, обратитесь к администратору.
With the PVE 8.0 major release, the scope of
non-"Permissions.Modify"-based ACL update privileges were reduced (so
that users with for example, VM.Allocate on a VM could only delegate
their own privileges, but not arbitrary other ones). that additional
logic had a wrong guard and was accidentally triggered for calls where
the user had the "Permissions.Modify" privilege on the modified ACL
path, but without propagation set.
A user with "Permissions.Modify" on a path should be able to set
arbitrary ACLs for that path, even without propagation.
Reported on the forum: https://forum.proxmox.com/threads/151032/
Fixes: 46bfd59 ("acls: restrict less-privileged ACL modifications")
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Stable sorting in user.cfg config file allows tracking changes by
checking into git or when using automation like ansible.
Signed-off-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Tested-by: Folge Gleumes <f.gleumes@proxmox.com>
This was reported by a user in the forum [0].
The cause was that the user-* standard options were not registered
when the sync was called from the scheduler, resulting in the
following error:
pvescheduler[2849]: skipping attribute mapping 'cn'->'comment' for user 'test@samba0' - no such standard option 'user-comment'
Fix this by simply importing the PVE::API2::User module, thus ensuring
the options get registered.
[0] https://forum.proxmox.com/threads/ldap-integration-comment-email-first-name-lastname.143490/
Fixes: cb93636 ("LDAP sync: improve validation of synced attributes")
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
Add a new 'confirmation-password' parameter to the change-password
endpoint and require non-root users to confirm their passwords.
Doing so avoids that an attacker that has direct access to a computer
where a user is logged in to the PVE interface can change the password
of said user and thus either prolong their possibility to attack,
and/or create a denial of service situation, where the original user
cannot login into the PVE host using their old credentials.
Note that this might sound worse than it is, as for this attack to
work the attacker needs either:
- physical access to an unlocked computer that is currently logged in
to a PVE host
- having taken over such a computer already through some unrelated
vulnerability
As these required pre-conditions are pretty big implications, which
allow (temporary) access to all of the resources (including PVE ones)
that the user can control, we see this as slight improvement that
won't hurt, might protect one in some specific cases that is simply
too cheap not to do.
For now we avoid additional confirmation through a second factor,
as that is a much higher complexity without that much gain, and some
forms like (unauthenticated) button press on a WebAuthn token or the
TOTP code would be easy to circumvent in the physical access case and
in the local access case one might be able to MITM themselves too.
Reported-by: Wouter Arts <security@wth-security.nl>
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
[ TL: extend commit message ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
since the upcoming use case in change_password uses the returned $ruid
and the parameter is called 'confirmation-password' there
also generalize the error so it does not mention TFA
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
now called "reauth_user_for_user_modification" since we
re-authenticate the current user while they are trying to modify
their (or others') password/tfa settings
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
We have some API endpoints that can access the network from the POV of
a Proxmox VE node, like e.g., the one for downloading a template/ISO
image directly to a PVE storage from an HTTP URL, and the matching
query-url-metadata that makes this functionality much more convenient
to use in the UI. But the downside of such calls is naturally that
they basically allow to scan the whole network via HTTP URLs, and
potentially even download some image that the user should not have
access to and adding to a VM that the user controls.
Due to that we limited the exposure of those API endpoints to
Sys.Modify on / (in addition to e.g. basic storage privs) for the
initial addition of the feature, as we were not sure about user
adoption and if a separate privilege could be justified.
Since we got a handful requests like #5254 this justification is now
met, so add a 'Sys.AccessNetwork' privilege.
That name should make it clear that having that privilege will allow
access to the network and the sys(tem) prefix should underline that
it's about the host systems network. Add it such, that it will only be
available for the most powerful of our built-in special roles, namely
the Administration one, besides naturally the all-powerful root@pam
special user.
Admins can then e.g. create new roles that include Sys.AccessNetwork
and Datastore.AllocateTemplate which can then be used for allowing
automation to download images while adhering to the Least Privilege
Principle.
Buglink: https://bugzilla.proxmox.com/show_bug.cgi?id=5254
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
for a better overview and to allow slightly easier tracking of any
change, like adding a new privilege.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Restrict the acr-value regex a little bit so as to align the behavior
with PBS. The openid documentation says that the acr-value *should* be
an URI [0]. Added a regex that loosely disallows some of the reserved
URI characters specified in the RFC [1].
Values like:
* "urn:mace:incommon:iap:silver"
* "urn:comsolve.nl:idp:contract:rba:location"
SHOULD work, but values like:
* "urn:#ace:incommon:iap:silver"
* "urn:"omsolve.nl:idp:contract:rba:location"
should NOT work.
This is related to the fix [2] for bug #5190 in PBS, but different as
there we had to make the verifier more flexible, whereas here we make
it stricter – mostly to have both projects aligned to avoid confusion.
[0]: https://openid.net/specs/openid-connect-core-1_0.html
[1]: https://www.rfc-editor.org/rfc/rfc2396.txt
[2]: https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=e0222ce83c28397d493c70825e873943c1223c67
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
For email the reasoning is:
> In addition to restrictions on syntax, there is a length limit on
> email addresses. That limit is a maximum of 64 characters (octets)
> in the "local part" (before the "@") and a maximum of 255
> characters (octets) in the domain part (after the "@") for a total
> length of 320 characters. However, there is a restriction in RFC
> 2821 on the length of an address in MAIL and RCPT commands of 254
> characters. Since addresses that do not fit in those fields are
> not normally useful, the upper limit on address lengths should
> normally be considered to be 254.
-- https://www.rfc-editor.org/errata_search.php?rfc=3696&eid=1690
And for user-comments, we normally show those as single line and using
2048 bytes as maximum, while also a rather arbitrary number it allows
for about 2.5 times more users on a system (full name + comment can be
up to 4 KiB vs 10 KiB), and we can re-raise this relatively easily
again if there are somewhat reasonable complaints.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
The user.cfg file resides on the cluster filesystem where files have
a maximum allowed size (currently 1 MiB).
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Should not matter for now, but better to to catch explicitly, e.g., if
anybody ever adds new attributes or changes the default options names
without adapting this too.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Build the set of valid target attributes on the fly by using the
existing ldap => ours mapping. This avoids that one needs to adapt
both lists when changing this, which even though it should be caught
on testing, is needlessly adding friction.
The is-known-target-attr check could never trigger as this was already
checked in the parent before even calling the verify method, so just
remove it.
Rename the `verify_sync_attribute` to `verify_sync_attribute_value` to
clarify that it really only checks the value of an attribute, not the
attribute (key) itself.
As a side-benefit, this also makes the code shorter and avoids a
permanent global variable using up (a tiny amount of) space.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
and skip the ones not fitting our schema, while warning the user about
them.
Also warns the user if the specified 'sync_attributes' mapping
contains entries for attributes that don't exist, e.g.
'enabled=active' (since the property on PVE side is called 'enable').
For the 'enable' property, any value coming from the server led to the
user being enabled, even "0", because it is a string. This is not
changed by this patch, by not trying to validate or parse a boolean.
In get_users(), the username is also set in the returned hash, but
without the realm. This doesn't seem to be necessary for syncing,
because the username with the realm is used as a hash key and that's
what's relied upon when updating the config. But the tests require it
to be set, so that is not changed by this patch either.
Relies on the user properties (other than username) to be standard
options called 'user-XYZ'. Could be improved by moving the schema for
user properties from the API module to a module that can be accessed
by both API and plugin here and creating a helper for accessing it.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
While nowadays, most entries should be just 'x', there can also still
be legacy entries with 'x!u2f', 'x!yubico' and base32 encoded secrets.
For example, some users might be syncing them from LDAP.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
this file is installed by a sub-dir Makefile, it does not exist in
src/PVE/API2.
the error is not fatal, but printed during build:
install: cannot stat 'RealmSync.pm': No such file or directory
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
else it's not actually possible to define ACLs on them, which means they are
effectively root only instead of allowing their intended permission scheme.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
/pools is not an allowed ACL path, so this would add a bogus entry into the
effective permissions in case something got propagated from /.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Removes the dreaded DN regex, instead introducing a optional
connect/bind check on creation/update, aligning it with the way PBS does
it.
Additionally, it has the benefit that instead of letting a sync fail on
the first try due to e.g. bad bind credentials, it gives the user some
direct feedback when trying to add/update a LDAP realm, if enabled.
Should be rather a corner case, but it's the easiest way for us to
accomodate and the most versatile for users needing this.
This is part of the result of a previous discussion [0], and the same
approach is already implemented for PBS [1].
[0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056839.html
[1] https://lists.proxmox.com/pipermail/pbs-devel/2023-June/006237.html
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Previously, if one tried to only update e.g. the bind password for an
LDAP realm, it would fail with an rather unhelpful error message:
# pveum realm modify ldap -password foo
update auth server failed: no options specified
The root cause was that the `password` parameter was removed early from
the parameter object, which than would fail the check whether it is
empty or not.
Thus, additionally check if only `password` was specified and if so,
allow it.
Reported-by: Friedrich Weber <f.weber@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
Before 0f3d14d6 ("auth: tfa: read tfa.cfg also if the user.cfg entry
has no "x" marker"), `user_get_tfa` failed if the realm required TFA,
but the user's `keys` attribute was empty. Since 0f3d14d6,
`user_get_tfa` fails if the realm requires TFA, but neither user.cfg
nor tfa.cfg define any second factors for that user.
However, both before and after 0f3d14d6, a realm that requires TOTP
allows a user to login without a second factor if they have at least
one configured factor in tfa.cfg and all factors are disabled -- for
example if they have only a disabled TOTP factor. This behavior is
unwanted, as users can then circumvent the realm-mandated TFA
requirement by disabling their own TOTP factor.
This happens because a user with a disabled TOTP factor in tfa.cfg
passes the check in `user_get_tfa`. Hence, `authenticate_2nd_new_do`
proceeds to call `authentication_challenge`, which does not generate a
challenge (and returns undef) because the user has no enabled factors.
Consequently, `authenticate_2nd_new_do` returns undef and allows login
without a second factor.
Note that this does not happen for realms that require Yubico TFA,
because for these realms, `authenticate_2nd_new_do` does not call
`authentication_challenge` and instead generates a challenge in any
case, regardless of whether the user has enabled Yubico factors or
not.
This patch fixes the issue by moving the check out of `user_get_tfa`,
and instead letting `authenticate_2nd_new_do` fail if the realm
requires TFA but `authentication_challenge` generates no challenge
(returns undef). This also saves a call to `api_list_user_tfa` that
was introduced in 0f3d14d6.
This patch still allows users to login with a recovery key to a realm
that requires TFA , which is the intended behavior.
Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
Alternatively we could potentially move the realm-tfa check to after
`authentication_challenge`.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Previously, `user_get_tfa` read the `keys` user attribute from
user.cfg to determine whether a user has second factors configured.
`keys` could contain TOTP secrets or Yubico key IDs (for realms that
require TFA), or the marker "x" to signify that second factors are
defined in tfa.cfg, in which case `user_get_tfa` would additionally
read tfa.cfg.
However, syncing an LDAP realm with `remove-vanished=properties`
erases the `keys` attribute, and thus the "x" marker (unless custom
`sync_attributes` with a mapping for `keys` are defined). This would
allow TFA-enabled users to log in without a second factor after a
realm sync. This issue was first reported in the forum [1].
To fix this issue, `user_get_tfa` now reads tfa.cfg unconditionally,
and thus independently of the value of `keys`. In other words, the "x"
marker is now irrelevant for authentication. The reasoning for this
change is that most current setups define second factors in tfa.cfg
anyway.
Special care is needed to avoid breaking realms that require TFA: In
that case, `user_get_tfa` must fail authentication if neither user.cfg
nor tfa.cfg define any second factors.
This patch changes the behavior of a hypothetical (and not officially
supported) LDAP realm setup in which `sync_attributes: keys=attr` and
`remove-vanished=properties` is used to maintain `keys` in the LDAP
directory. In such a setup, an admin could enable/disable TFA for a
user who has an enabled second factor in tfa.cfg by editing their LDAP
entry and switching between "x" and "". With this patch, TFA is always
enabled for that user.
This patch makes the "x" marker irrelevant for authentication, but PVE
still *writes* it if the user has second factors configured in
tfa.cfg. This behavior is kept for now to avoid issues in cluster
upgrade scenarios, where some nodes that still rely on the "x" marker
could allow logins without a second factor.
[1] https://forum.proxmox.com/threads/130440/
Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
All nodes should be new enough, especially as this is understood
since pve-manager 7.0-15 and users must upgrade to 7.4 before
upgrading to Proxmox VE 8
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
The `allowtoken` property is a total, unconditional block on using
API tokens on an endpoint. We reserve those only for a limited set of
security critical endpoints like changing passwords or second
factors, or creating a (cookie) ticket, which are exempt from this
limitations, so require to have limited access to them too.
Anyhow, listing and getting TFA entries for users, where the API
token has the correct permissions granted, is not critical, as the
API token cannot gain more permissions than they have from that
info, so drop the total block on those GET methods.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
