Thomas Lamprecht [Wed, 28 Feb 2024 14:42:20 +0000 (15:42 +0100)]
bump version to 8.1.2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Mon, 19 Feb 2024 15:25:51 +0000 (16:25 +0100)]
add Sys.AccessNetwork privilege
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>
Thomas Lamprecht [Mon, 19 Feb 2024 14:12:22 +0000 (15:12 +0100)]
tests: split long expected-permission list over multiple lines
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>
Thomas Lamprecht [Mon, 19 Feb 2024 13:40:10 +0000 (14:40 +0100)]
special roles: code-style improvements for generation of special roles
no semantic change intended
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 8 Feb 2024 18:03:31 +0000 (19:03 +0100)]
bump version to 8.1.1
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 8 Feb 2024 18:03:10 +0000 (19:03 +0100)]
LDAP sync: fix-up assembling valid attribute set
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 8 Feb 2024 16:58:05 +0000 (17:58 +0100)]
bump version to 8.1.0
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Gabriel Goller [Tue, 6 Feb 2024 10:11:01 +0000 (11:11 +0100)]
oidc: enforce generic URI regex for the ACR value
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>
Thomas Lamprecht [Thu, 8 Feb 2024 16:31:04 +0000 (17:31 +0100)]
api: user: limit email to 254 characters and user comment to 2048
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>
Fiona Ebner [Thu, 8 Feb 2024 09:45:41 +0000 (10:45 +0100)]
api: user: limit maximum length for certain properties
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>
Thomas Lamprecht [Thu, 8 Feb 2024 12:13:45 +0000 (13:13 +0100)]
LDAP sync: bail if there is no schema to verify an attribute's value
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>
Thomas Lamprecht [Thu, 8 Feb 2024 11:10:31 +0000 (12:10 +0100)]
LDAP sync: build valid-target-attribute list on the fly to avoid coupling
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>
Fiona Ebner [Thu, 8 Feb 2024 09:45:40 +0000 (10:45 +0100)]
LDAP sync: improve validation of synced attributes
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>
Fiona Ebner [Thu, 8 Feb 2024 09:45:39 +0000 (10:45 +0100)]
api: user: add pattern for user keys option
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>
Fabian Grünbichler [Thu, 7 Dec 2023 11:36:40 +0000 (12:36 +0100)]
build: fix file list
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>
Wolfgang Bumiller [Mon, 20 Nov 2023 11:24:32 +0000 (12:24 +0100)]
bump version to 8.0.7
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Fabian Grünbichler [Mon, 20 Nov 2023 07:22:40 +0000 (08:22 +0100)]
pools: record parent/subpool information
and ensure a missing intermediate pool exists at all times.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Mon, 20 Nov 2023 07:22:39 +0000 (08:22 +0100)]
fix #1148: allow up to three levels of pool nesting
with ACLs being inherited along the pool hierarchy.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Fri, 17 Nov 2023 07:27:08 +0000 (08:27 +0100)]
bump version to 8.0.6
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Wed, 8 Nov 2023 06:55:17 +0000 (07:55 +0100)]
acl: add missing SDN ACL paths to allowed list
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>
Fabian Grünbichler [Wed, 8 Nov 2023 10:29:29 +0000 (11:29 +0100)]
perms: fix wrong /pools entry in default set of ACL paths
/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>
Wolfgang Bumiller [Fri, 11 Aug 2023 11:35:37 +0000 (13:35 +0200)]
bump version to 8.0.5
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Fri, 11 Aug 2023 11:30:13 +0000 (13:30 +0200)]
fixup comments about config locks
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Christoph Heiss [Thu, 10 Aug 2023 12:37:07 +0000 (14:37 +0200)]
ldap: add opt-in `check-connection` param to perform a bind check
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>
Wolfgang Bumiller [Fri, 11 Aug 2023 11:30:47 +0000 (13:30 +0200)]
bump pve-common dependency to 8.0.8
required for next patch introducing the check-connection parameter for
ldap based realms
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Christoph Heiss [Thu, 20 Jul 2023 14:48:47 +0000 (16:48 +0200)]
api: domains: fix setting only the realm authentication password
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>
Wolfgang Bumiller [Thu, 20 Jul 2023 08:59:56 +0000 (10:59 +0200)]
bump version to 8.0.4
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Friedrich Weber [Wed, 19 Jul 2023 15:38:04 +0000 (17:38 +0200)]
auth: tfa: fail if realm requires TFA but no challenge is generated
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>
Wolfgang Bumiller [Fri, 14 Jul 2023 12:16:19 +0000 (14:16 +0200)]
add fixme comments about pending pve-rs improvements
Alternatively we could potentially move the realm-tfa check to after
`authentication_challenge`.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Friedrich Weber [Fri, 14 Jul 2023 11:49:50 +0000 (13:49 +0200)]
auth: tfa: read tfa.cfg also if the user.cfg entry has no "x" marker
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>
Thomas Lamprecht [Wed, 21 Jun 2023 17:45:32 +0000 (19:45 +0200)]
bump version to 8.0.3
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 21 Jun 2023 17:43:37 +0000 (19:43 +0200)]
drop assert_new_tfa_config_available for Proxmox VE 8
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>
Thomas Lamprecht [Wed, 21 Jun 2023 17:41:31 +0000 (19:41 +0200)]
pveum: list tfa: sort by user ID
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 21 Jun 2023 17:27:41 +0000 (19:27 +0200)]
pveum: list tfa: recovery keys have no descriptions
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 21 Jun 2023 16:13:58 +0000 (18:13 +0200)]
bump version to 8.0.2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 15 Jun 2023 14:55:41 +0000 (16:55 +0200)]
api: tfa: add missing links for child-routes
making this more useable in things like pvesh or the HTML formatter
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 15 Jun 2023 07:23:33 +0000 (09:23 +0200)]
api: tfa: don't block tokens from viewing and list TFA entries
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>
Thomas Lamprecht [Wed, 14 Jun 2023 14:07:55 +0000 (16:07 +0200)]
api: users: sort groups
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 9 Jun 2023 14:12:05 +0000 (16:12 +0200)]
bump version to 8.0.1
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 9 Jun 2023 14:06:43 +0000 (16:06 +0200)]
tfa: cope with native versions in cluster version check
Reported-by: Friedrich Weber <f.weber@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 9 Jun 2023 08:14:36 +0000 (10:14 +0200)]
bump version to 8.0.0
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 8 Jun 2023 07:31:19 +0000 (09:31 +0200)]
api: roles: forbid creatin new roles starting with "PVE" namespace
makes our reasoning when adding new top-level privileges way easier
in the future.
We already had two major upgrades with role additions where we had to
add special checks in the upgrade script and breaking changes, so
let's reserve any role starting with PVE (case-insensitive to avoid
confusion potential) and forbid creating those via API.
We might also think about letting the config parser choke on that, as
otherwise one could still create them via editing the config
manually.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 8 Jun 2023 07:29:25 +0000 (09:29 +0200)]
api: roles: cleanup imports
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Thu, 8 Jun 2023 07:22:00 +0000 (09:22 +0200)]
api: roles: whitespace and indendation clean-ups
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 7 Jun 2023 17:06:58 +0000 (19:06 +0200)]
bump version to 8.0.0~3
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 7 Jun 2023 17:12:04 +0000 (19:12 +0200)]
tests: adapt new test for admin ACL with mapping privs
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Tue, 6 Jun 2023 13:52:00 +0000 (15:52 +0200)]
add privileges and paths for cluster resource mapping
uses the privileges:
Mapping.Use
Mapping.Modify
Mapping.Audit
on /mapping/{TYPE}/{id}
so that we can assign privileges on resource level
this will generate new roles (PVEMappingUser, PVEMappingAdmin,
PVEMappingAuditor)
note that every user with Permissions.Modify on '/' and propagate can add these
new roles to themselves
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Alexandre Derumier [Tue, 6 Jun 2023 13:19:25 +0000 (15:19 +0200)]
add new SDN.use privilege in PVESDNUser role
Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
FG: fix test
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Wed, 7 Jun 2023 09:34:33 +0000 (11:34 +0200)]
check_sdn_bridge: check bridge first
it's cheap, so let's use it for early returning
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Wed, 7 Jun 2023 09:33:39 +0000 (11:33 +0200)]
check_sdn_bridge: correctly handle noerr
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Alexandre Derumier [Tue, 6 Jun 2023 13:19:24 +0000 (15:19 +0200)]
rpcenvironnment: add check_sdn_bridge
check if user have access to 1 vlan of the bridge
or the bridge itself
Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
Alexandre Derumier [Tue, 6 Jun 2023 13:19:18 +0000 (15:19 +0200)]
access control: add /sdn/zones/<zone>/<vnet>/<vlan> path
Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
FG: add missing /sdn/zones path
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Alexandre Derumier [Mon, 27 Mar 2023 10:18:20 +0000 (12:18 +0200)]
rpcenv: api permission heuristic: query Sys.Modify for root ACL-path
Ensures that we can use this in the capabilities heuristic check in
the web UI
Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
Thomas Lamprecht [Wed, 7 Jun 2023 09:34:33 +0000 (11:34 +0200)]
bump version to 8.0.0~2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Fabian Grünbichler [Mon, 5 Jun 2023 14:21:37 +0000 (16:21 +0200)]
acls: restrict less-privileged ACL modifications
there are currently three possibilities to modify ACLs without the
'Permissions.Modify' privilege in PVE::RPCEnvironment::check_perm_modify:
if ($path =~ m|^/storage/.+$|) {
push @$testperms, 'Datastore.Allocate';
} elsif ($path =~ m|^/vms/.+$|) {
push @$testperms, 'VM.Allocate';
} elsif ($path =~ m|^/pool/.+$|) {
push @$testperms, 'Pool.Allocate';
}
lock those down by only allowing the currently authenticated user to hand out a
subset of their own privileges, never more.
for example, this still allows a PVEVMAdmin to create ACLs for other
users/tokens with PVEVMUser (on '/vm/XXX'), but not with Administrator or
PVEPermAdmin.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Fabian Grünbichler [Mon, 5 Jun 2023 14:21:36 +0000 (16:21 +0200)]
roles: restrict Permissions.Modify to Administrator
to reduce the chances of accidentally handing out privilege modification
privileges. the old default setup of having Permissions.Modify in PVESysAdmin
and PVEAdmin weakened the distinction between those roles and Administrator.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Thomas Lamprecht [Wed, 7 Jun 2023 07:37:46 +0000 (09:37 +0200)]
fix module namespace of realm sync API
it isn't mounted in PVE::API2::AccessControl and it doesn't lives
anywhere in /access, so using that is just confusing.
Both, API and backend could simply move to manager, but as we already
got an api package here and it does somewhat fits into the topic lets
keep it here for now.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 7 Jun 2023 07:33:16 +0000 (09:33 +0200)]
api: realm sync: die early if no job options
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 7 Jun 2023 07:32:26 +0000 (09:32 +0200)]
api: realm sync: code cleanups
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 7 Jun 2023 07:31:39 +0000 (09:31 +0200)]
realm sync: cleanup imports
with: perlimports -i --no-padding
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Dominik Csapak [Tue, 17 Jan 2023 11:46:54 +0000 (12:46 +0100)]
add realm-sync plugin for jobs and CRUD api for realm-sync-jobs
to be able to define automated jobs that sync ldap/ad
The jobs plugin contains special handling when no node is given, since
we only want it to run on a single node when that triggers. For that,
we save a statefile in /etc/pve/priv/jobs/ which contains the
node/time/upid of the node that runs the job. The first node that
is able to lock the realm (via cfs_lock_domain) "wins" and may
sync from the ldap.
in case a specific node was selected, this is omitted and the Jobs
handling will not let it run on other nodes anyway
the API part is our usual sectionconfig CRUD api, but specialized
for the specific type of job.
the api will be at /cluster/jobs/realm-sync
(this must be done in pve-manager)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
[ T: resolve merge conflict due to packaging/source split ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Tue, 6 Jun 2023 09:18:56 +0000 (11:18 +0200)]
api: user index: only include existing tfa lock flags
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 5 Jun 2023 12:52:39 +0000 (14:52 +0200)]
bump version to 8.0.0~1
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Fri, 2 Jun 2023 09:28:34 +0000 (11:28 +0200)]
api: include tfa lock status in user list
this means /access/users is now a 'protected' call to get
access to 'priv/tfa.cfg'
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Thu, 1 Jun 2023 10:26:10 +0000 (12:26 +0200)]
tfa: update list_tfa return schema
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 30 May 2023 09:55:08 +0000 (11:55 +0200)]
cli: add 'tfa unlock' command
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 30 May 2023 11:39:15 +0000 (13:39 +0200)]
api: add /access/users/<userid>/unlock-tfa api call
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 16 May 2023 11:43:53 +0000 (13:43 +0200)]
tfa: enable lockout of users via tfa.cfg
This will be accompanied by a change in pve-rs to finally
enable this.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 5 Jun 2023 10:59:26 +0000 (12:59 +0200)]
bump pve-rs dep to 0.8.3
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Thu, 1 Jun 2023 09:36:54 +0000 (11:36 +0200)]
pam: set PAM_RHOST
This allows pam modules to restrict users by host. For
instance, you could restrict root@pam to only 127.0.0.1.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 30 May 2023 11:00:04 +0000 (13:00 +0200)]
cli: add 'tfa list' command
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 16 May 2023 11:56:13 +0000 (13:56 +0200)]
drop support for old login API
The new-format parameter for the ticket call is now ignored
and assumed 1.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 16 May 2023 11:48:45 +0000 (13:48 +0200)]
api: drop old verify_tfa api call
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 22 May 2023 08:27:15 +0000 (10:27 +0200)]
depend on pve-rs 0.8.0
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Tue, 16 May 2023 12:23:15 +0000 (14:23 +0200)]
add anyevent to build deps
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 10:06:18 +0000 (12:06 +0200)]
bump version to 7.99.0
use a pre-release like version as we got some breaking changes
planned for access control, so might be nice to get (most of) them in
a 8.0.0 for simpler versioned dependencies (>= 8~), but it's also
just a bit of an experiment to see how doing such things plays out,
in the end we can cope with whatever versioning for dependency as bug
fixes might make it necessary to have a more specific version
boundary anyway.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 10:38:00 +0000 (12:38 +0200)]
d/control: raise standards version compliance to 4.6.2
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 09:18:56 +0000 (11:18 +0200)]
d/control: define compat level via build-depends and raise to 13
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 10:34:19 +0000 (12:34 +0200)]
oathkeygen: modernize and improve error handling slightly
Note that this could also just be documented doing:
dd if=/dev/urandom bs=1 count=10 2>/dev/null | base32
And using TOTP apps that can scan QR codes is much better UX anyway,
but its to trivial to bother deprecating it now and we'd still depend
on libmime-base32-perl, so really nothing gained in removing or
rewriting in shell..
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 10:06:06 +0000 (12:06 +0200)]
buildsys: add sbuild convenience target
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 10:05:12 +0000 (12:05 +0200)]
buildsys: cleanup and expand clean target
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 09:47:08 +0000 (11:47 +0200)]
src makefiles: convert to use simple parenthesis
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 09:46:27 +0000 (11:46 +0200)]
buildsys: drop useless packaging variable/includes in src
and allow overriding the PACKAGE variable, making it actually useful
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 09:42:21 +0000 (11:42 +0200)]
buildsys: rework clean target, avoid doc-gen one
1. this really doesn't change often
2. the synopsis and opts should be in the owner repo anyway
3. the original one simply deleted all *.adoc files, far to
aggressive
Avoids pve-docs dependency for building the DSC (without having to
pass the ugly no-pre-clean option).
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 09:19:16 +0000 (11:19 +0200)]
buildsys: derive upload dist automatically
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 09:18:36 +0000 (11:18 +0200)]
buildsys: don't pass arch for an arch: all package
it was wrong too anyway, if, one would need to use the
$(DEB_HOST_ARCH), as that's the one package is built for.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 08:53:20 +0000 (10:53 +0200)]
use combined dpkg packaging variable makefile fragment
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sun, 21 May 2023 08:52:58 +0000 (10:52 +0200)]
makefile: convert to use simple parenthesis
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Wolfgang Bumiller [Tue, 16 May 2023 11:33:54 +0000 (13:33 +0200)]
bump version to 7.4-3
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Wolfgang Bumiller [Mon, 15 May 2023 07:06:28 +0000 (09:06 +0200)]
use TFA authentication api v2
Previously `authentication_verify` just `die`d on error and
would only return a boolean whether `priv/tfa.cfg` needs
updating as a positive result.
Since we want to support locking TOTP as well as a general
TFA lock-out via the config, we also want to be able to tell
when this occurs. Most of it is handled by the TFA rust
crate already, but notifying users needs to be done on this
end instead.
In pve-rs we now have a different API for this:
`authentication_verify2`, which, instead of die()ing on
errors, always returns a hash containing the result as well
as the flags 'tfa-limit-reached' and 'totp-limit-reached'
which, if set, tell us to notify the user.
However, doing so will introduce new fields in the
`priv/tfa.cfg` in a struct marked as `deny_unknown_fields`,
so in a cluster, the limits & notification handling should
only be done once we can be sure that all nodes are up to
date.
These fields are only introduced on login errors, so for
now, handle a failed result early without saving
`priv/tfa.cfg`.
The only case where saving the file was previously required
was when *successfully* logging in with a recovery key, by
which we cannot be reaching a limit, so this should still be
safe.
Once we can validate that all cluster nodes are up to date,
we can implement the notification system.
A commented-out code structure for this is included in this
patch.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Thomas Lamprecht [Tue, 11 Apr 2023 13:45:38 +0000 (15:45 +0200)]
fix various perlcritic lints
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Tue, 11 Apr 2023 13:13:43 +0000 (15:13 +0200)]
oathkeygen: code style fixes
we probably should just deprecate this altogether as it could be
worked around by doing a simple
dd if=/dev/urandom bs=1 count=10 status=none | base32
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
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>