* [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured @ 2022-09-14 13:42 Dominik Csapak 2022-09-15 7:01 ` Fabian Grünbichler 2022-09-15 10:43 ` Thomas Lamprecht 0 siblings, 2 replies; 6+ messages in thread From: Dominik Csapak @ 2022-09-14 13:42 UTC (permalink / raw) To: pve-devel With change to our new tfa mechanism, we now lock the tfa config when verifying the second factor and when creating the challenge for it that makes sense, since at least one tfa type can change the config (recovery keys must be deleted from there). The downside is that we cannot authenticate users anymore without quorum (since locking requires write access to pmxcfs), even for users without tfa configured (and also for clusters without any tfa configured at all). With this patch, we loosen that restriction a bit, by checking if the user has tfa configured outside the lock. We still query it again inside the lock to get the current config inside the lock again. (slightly out of the diff context) There is a minimal (IMHO unimportant) race here: if a user is logging in and for this user tfa is configured simultaneously, it may happen that during the first check tfa is still not present. This is not a real problem though, since a logged in user will not be logged out when a tfa is configured, so it's the same as when the user would have logged in before the tfa is being configured. There were quite a bit confused users that ran into that, and preventing them from logging in at all because of a not quorate server (when there is not a good technical reason for it) seems bad It's still not possible to login when tfa is enabled though, but at least for simple setups it should be a bit better Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> --- src/PVE/AccessControl.pm | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm index c32dcc3..52ad84b 100644 --- a/src/PVE/AccessControl.pm +++ b/src/PVE/AccessControl.pm @@ -790,6 +790,12 @@ sub authenticate_2nd_old : prototype($$$) { sub authenticate_2nd_new : prototype($$$$) { my ($username, $realm, $otp, $tfa_challenge) = @_; + my ($tfa_cfg) = user_get_tfa($username, $realm, 1); + + if (!defined($tfa_cfg)) { + return undef; + } + my $result = lock_tfa_config(sub { my ($tfa_cfg, $realm_tfa) = user_get_tfa($username, $realm, 1); -- 2.30.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured 2022-09-14 13:42 [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured Dominik Csapak @ 2022-09-15 7:01 ` Fabian Grünbichler 2022-09-15 10:43 ` Thomas Lamprecht 1 sibling, 0 replies; 6+ messages in thread From: Fabian Grünbichler @ 2022-09-15 7:01 UTC (permalink / raw) To: Proxmox VE development discussion; +Cc: w.bumiller I think this is https://bugzilla.proxmox.com/show_bug.cgi?id=3739 ;) @wolfgang could you take a look at this? On September 14, 2022 3:42 pm, Dominik Csapak wrote: > With change to our new tfa mechanism, we now lock the tfa config > when verifying the second factor and when creating the challenge for it > that makes sense, since at least one tfa type can change the config > (recovery keys must be deleted from there). > > The downside is that we cannot authenticate users anymore without quorum > (since locking requires write access to pmxcfs), even for users without > tfa configured (and also for clusters without any tfa configured at all). > > With this patch, we loosen that restriction a bit, by checking if the > user has tfa configured outside the lock. We still query it again > inside the lock to get the current config inside the lock again. > (slightly out of the diff context) > > There is a minimal (IMHO unimportant) race here: > if a user is logging in and for this user tfa is configured > simultaneously, it may happen that during the first check tfa is still > not present. > > This is not a real problem though, since a logged in user will not be > logged out when a tfa is configured, so it's the same as when the user > would have logged in before the tfa is being configured. > > There were quite a bit confused users that ran into that, and preventing > them from logging in at all because of a not quorate server (when > there is not a good technical reason for it) seems bad > > It's still not possible to login when tfa is enabled though, but at > least for simple setups it should be a bit better > > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> > --- > src/PVE/AccessControl.pm | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm > index c32dcc3..52ad84b 100644 > --- a/src/PVE/AccessControl.pm > +++ b/src/PVE/AccessControl.pm > @@ -790,6 +790,12 @@ sub authenticate_2nd_old : prototype($$$) { > sub authenticate_2nd_new : prototype($$$$) { > my ($username, $realm, $otp, $tfa_challenge) = @_; > > + my ($tfa_cfg) = user_get_tfa($username, $realm, 1); > + > + if (!defined($tfa_cfg)) { > + return undef; > + } > + > my $result = lock_tfa_config(sub { > my ($tfa_cfg, $realm_tfa) = user_get_tfa($username, $realm, 1); > > -- > 2.30.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured 2022-09-14 13:42 [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured Dominik Csapak 2022-09-15 7:01 ` Fabian Grünbichler @ 2022-09-15 10:43 ` Thomas Lamprecht 2022-09-15 12:40 ` Dominik Csapak 1 sibling, 1 reply; 6+ messages in thread From: Thomas Lamprecht @ 2022-09-15 10:43 UTC (permalink / raw) To: Proxmox VE development discussion, Dominik Csapak Am 14/09/2022 um 15:42 schrieb Dominik Csapak: > The downside is that we cannot authenticate users anymore without quorum > (since locking requires write access to pmxcfs), even for users without > tfa configured (and also for clusters without any tfa configured at all) question is more if we should disallow login on unquorate clusters for all but root@pam, as for all others you cannot be sure if they still got the permissions and for the pve realm the credentials are still correct, or if the non-existing TFA entry is still up-to-date (the quorate partition could have TFA configured for that user since cluster split). root@pam is a hard-coded super admin and verified via PAM, which normally should be pmxcfs, and thus quorum, independent, at least if nobody was crazy enough to link /etc/shadow to a file in pmxcfs or wrote a PAM that works on pmxcfs info in other ways. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured 2022-09-15 10:43 ` Thomas Lamprecht @ 2022-09-15 12:40 ` Dominik Csapak 2022-09-15 12:53 ` Thomas Lamprecht 0 siblings, 1 reply; 6+ messages in thread From: Dominik Csapak @ 2022-09-15 12:40 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion On 9/15/22 12:43, Thomas Lamprecht wrote: > Am 14/09/2022 um 15:42 schrieb Dominik Csapak: >> The downside is that we cannot authenticate users anymore without quorum >> (since locking requires write access to pmxcfs), even for users without >> tfa configured (and also for clusters without any tfa configured at all) > > question is more if we should disallow login on unquorate clusters for all > but root@pam, as for all others you cannot be sure if they still got the > permissions and for the pve realm the credentials are still correct, or > if the non-existing TFA entry is still up-to-date (the quorate partition > could have TFA configured for that user since cluster split). fair point, but then we'd also have to deny api requests in general for non root@pam users when the cluster is not quorate, otherwise they can continue making requests with existing tickets (aside creating a new one) > > root@pam is a hard-coded super admin and verified via PAM, which normally > should be pmxcfs, and thus quorum, independent, at least if nobody was crazy > enough to link /etc/shadow to a file in pmxcfs or wrote a PAM that works > on pmxcfs info in other ways. AFAICS a user can't do anything on a non quorate cluster that wouldn't be possible over ssh for example ? since most operations already need the cluster to be quorate (aside from things such as network configuration, which might be the exact reason why the cluster is not quorate, so the admin/user wants to fix it) so imho @pam could be allowed in general, but for the other realms it's very much a design decision. just to note: before accidentally breaking such logins by locking the tfa config, we always let users login, regardless of cluster quorum state ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured 2022-09-15 12:40 ` Dominik Csapak @ 2022-09-15 12:53 ` Thomas Lamprecht 2022-09-15 13:12 ` Dominik Csapak 0 siblings, 1 reply; 6+ messages in thread From: Thomas Lamprecht @ 2022-09-15 12:53 UTC (permalink / raw) To: Dominik Csapak, Proxmox VE development discussion Am 15/09/2022 um 14:40 schrieb Dominik Csapak: > On 9/15/22 12:43, Thomas Lamprecht wrote: >> Am 14/09/2022 um 15:42 schrieb Dominik Csapak: >>> The downside is that we cannot authenticate users anymore without quorum >>> (since locking requires write access to pmxcfs), even for users without >>> tfa configured (and also for clusters without any tfa configured at all) >> >> question is more if we should disallow login on unquorate clusters for all >> but root@pam, as for all others you cannot be sure if they still got the >> permissions and for the pve realm the credentials are still correct, or >> if the non-existing TFA entry is still up-to-date (the quorate partition >> could have TFA configured for that user since cluster split). > > fair point, but then we'd also have to deny api requests in general > for non root@pam users when the cluster is not quorate, > otherwise they can continue making requests with existing tickets > (aside creating a new one) That is something rather different and bound to the ticket life time, they won't be able to renew the ticket or create new ones, so while the session continues to be available for maximal 2h for GET calls it has already the same limitations as new logins. IOW. if you want to see that as the same thing you need to adapt tickets to have a cluster state first that can actively be revoked on a per-ticket/user basis, as otherwise it'll always be "valid ticket = valid login". >> >> root@pam is a hard-coded super admin and verified via PAM, which normally >> should be pmxcfs, and thus quorum, independent, at least if nobody was crazy >> enough to link /etc/shadow to a file in pmxcfs or wrote a PAM that works >> on pmxcfs info in other ways. > > AFAICS a user can't do anything on a non quorate cluster that wouldn't be possible > over ssh for example ? since most operations already need the cluster to be quorate > (aside from things such as network configuration, which might be the exact reason > why the cluster is not quorate, so the admin/user wants to fix it) > > so imho @pam could be allowed in general, but for the other realms it's > very much a design decision. Standard pam accounts do not get their Proxmox VE access level if connected via SSH (or getting a shell in any other way), as the CLIHandler isn't user account aware, so not sure how that would be an argument for allowing all of pam (which may not even be allowed to SSH in or login (e.g., shell to /bin/false or the like, you just cannot (simply!) derive/know that) > > just to note: before accidentally breaking such logins by locking the tfa > config, we always let users login, regardless of cluster quorum state > yes I know, the question if that's right still stands, more secure is to not allow it, and iff, we could provide a datacenter.cfg option to allow such things with the (natural) warning that changes to that will only affected quorate notes. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured 2022-09-15 12:53 ` Thomas Lamprecht @ 2022-09-15 13:12 ` Dominik Csapak 0 siblings, 0 replies; 6+ messages in thread From: Dominik Csapak @ 2022-09-15 13:12 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion On 9/15/22 14:53, Thomas Lamprecht wrote: > Am 15/09/2022 um 14:40 schrieb Dominik Csapak: >> On 9/15/22 12:43, Thomas Lamprecht wrote: >>> Am 14/09/2022 um 15:42 schrieb Dominik Csapak: >>>> The downside is that we cannot authenticate users anymore without quorum >>>> (since locking requires write access to pmxcfs), even for users without >>>> tfa configured (and also for clusters without any tfa configured at all) >>> >>> question is more if we should disallow login on unquorate clusters for all >>> but root@pam, as for all others you cannot be sure if they still got the >>> permissions and for the pve realm the credentials are still correct, or >>> if the non-existing TFA entry is still up-to-date (the quorate partition >>> could have TFA configured for that user since cluster split). >> >> fair point, but then we'd also have to deny api requests in general >> for non root@pam users when the cluster is not quorate, >> otherwise they can continue making requests with existing tickets >> (aside creating a new one) > > That is something rather different and bound to the ticket life time, they > won't be able to renew the ticket or create new ones, so while the session > continues to be available for maximal 2h for GET calls it has already the > same limitations as new logins. IOW. if you want to see that as the same thing > you need to adapt tickets to have a cluster state first that can actively > be revoked on a per-ticket/user basis, as otherwise it'll always be "valid > ticket = valid login". we already do not allow requests for a user that gets disabled, so when a node loses connection to a cluster, the ticket is still valid there, while not valid on the remaining quorate nodes. i think this here is a similar scenario, since at the moment a node loses quorum, we cannot verify any property of the user anymore (since it could have changed) including the 'enabled/disabled' property, thus we'd have to prevent requests > >>> >>> root@pam is a hard-coded super admin and verified via PAM, which normally >>> should be pmxcfs, and thus quorum, independent, at least if nobody was crazy >>> enough to link /etc/shadow to a file in pmxcfs or wrote a PAM that works >>> on pmxcfs info in other ways. >> >> AFAICS a user can't do anything on a non quorate cluster that wouldn't be possible >> over ssh for example ? since most operations already need the cluster to be quorate >> (aside from things such as network configuration, which might be the exact reason >> why the cluster is not quorate, so the admin/user wants to fix it) >> >> so imho @pam could be allowed in general, but for the other realms it's >> very much a design decision. > > Standard pam accounts do not get their Proxmox VE access level if connected via > SSH (or getting a shell in any other way), as the CLIHandler isn't user account > aware, so not sure how that would be an argument for allowing all of pam (which > may not even be allowed to SSH in or login (e.g., shell to /bin/false or the like, > you just cannot (simply!) derive/know that ok, thats true ofc > >> >> just to note: before accidentally breaking such logins by locking the tfa >> config, we always let users login, regardless of cluster quorum state >> > > yes I know, the question if that's right still stands, more secure is to not > allow it, and iff, we could provide a datacenter.cfg option to allow such things > with the (natural) warning that changes to that will only affected quorate notes. mhmm while that could work, i think it does not serve the right purpose, since i think the times you want to (rightfully) login to non-quorate nodes is when you try to repair them and there are 3 scenarios here: * have it default on (not as secure) * have it default off, but the user have it enabled all the time (also not as secure) * have it default off, users leave it disabled -> they cannot enable it when they'd need to (when they want to login to a non-quorate node) maybe we'd want to make it an option per user? that way there can be one (or more) admin user(s) that can always login (we could make that default true for root@pam). all that said, it still does not solve the problem when that user has tfa enabled, so maybe we should also consider building a check which type of tfa the user has (configured/provided) and only lock it when it's necessary (so far only recovery keys fall under that) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-15 13:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-14 13:42 [pve-devel] [RFC PATCH access-control] loosen locking restriction for users without tfa configured Dominik Csapak 2022-09-15 7:01 ` Fabian Grünbichler 2022-09-15 10:43 ` Thomas Lamprecht 2022-09-15 12:40 ` Dominik Csapak 2022-09-15 12:53 ` Thomas Lamprecht 2022-09-15 13:12 ` Dominik Csapak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox