all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions
@ 2024-05-23 11:25 Shannon Sterz
  2024-05-23 11:46 ` Gabriel Goller
  2024-06-03  9:02 ` [pbs-devel] applied: " Wolfgang Bumiller
  0 siblings, 2 replies; 6+ messages in thread
From: Shannon Sterz @ 2024-05-23 11:25 UTC (permalink / raw)
  To: pbs-devel

currently we don't lock the shadow file when removing or storing a
password. by adding locking here we avoid a situation where storing
and/or removing a password concurrently could lead to a race
condition. in this scenario it is possible that a password isn't
persisted or a password isn't removed. we already do this for
the "token.shadow" file, so just use the same mechanism here.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 src/auth.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/auth.rs b/src/auth.rs
index 21468d4b..5134e41e 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -9,6 +9,7 @@ use std::pin::Pin;
 use anyhow::{bail, Error};
 use futures::Future;
 use once_cell::sync::{Lazy, OnceCell};
+use pbs_config::open_backup_lockfile;
 use proxmox_router::http_bail;
 use serde_json::json;
 
@@ -31,6 +32,7 @@ pub const TERM_PREFIX: &str = "PBSTERM";
 struct PbsAuthenticator;
 
 pub(crate) const SHADOW_CONFIG_FILENAME: &str = configdir!("/shadow.json");
+pub(crate) const SHADOW_LOCK_FILENAME: &str = configdir!("/shadow.json.lock");
 
 impl Authenticator for PbsAuthenticator {
     fn authenticate_user<'a>(
@@ -69,6 +71,8 @@ impl Authenticator for PbsAuthenticator {
         _client_ip: Option<&IpAddr>,
     ) -> Result<(), Error> {
         let enc_password = proxmox_sys::crypt::encrypt_pw(password)?;
+
+        let _guard = open_backup_lockfile(SHADOW_LOCK_FILENAME, None, true);
         let mut data = proxmox_sys::fs::file_get_json(SHADOW_CONFIG_FILENAME, Some(json!({})))?;
         data[username.as_str()] = enc_password.into();
 
@@ -85,6 +89,8 @@ impl Authenticator for PbsAuthenticator {
     }
 
     fn remove_password(&self, username: &UsernameRef) -> Result<(), Error> {
+        let _guard = open_backup_lockfile(SHADOW_LOCK_FILENAME, None, true);
+
         let mut data = proxmox_sys::fs::file_get_json(SHADOW_CONFIG_FILENAME, Some(json!({})))?;
         if let Some(map) = data.as_object_mut() {
             map.remove(username.as_str());
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions
  2024-05-23 11:25 [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions Shannon Sterz
@ 2024-05-23 11:46 ` Gabriel Goller
  2024-05-23 12:06   ` Shannon Sterz
  2024-06-03  9:00   ` Wolfgang Bumiller
  2024-06-03  9:02 ` [pbs-devel] applied: " Wolfgang Bumiller
  1 sibling, 2 replies; 6+ messages in thread
From: Gabriel Goller @ 2024-05-23 11:46 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion

On 23.05.2024 13:25, Shannon Sterz wrote:
>currently we don't lock the shadow file when removing or storing a
>password. by adding locking here we avoid a situation where storing
>and/or removing a password concurrently could lead to a race
>condition. in this scenario it is possible that a password isn't
>persisted or a password isn't removed. we already do this for
>the "token.shadow" file, so just use the same mechanism here.
>
>Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>

Is there any reason why the store_password function does not lock the
shadow.json file?


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions
  2024-05-23 11:46 ` Gabriel Goller
@ 2024-05-23 12:06   ` Shannon Sterz
  2024-06-03  9:00   ` Wolfgang Bumiller
  1 sibling, 0 replies; 6+ messages in thread
From: Shannon Sterz @ 2024-05-23 12:06 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion

On Thu May 23, 2024 at 1:46 PM CEST, Gabriel Goller wrote:
> On 23.05.2024 13:25, Shannon Sterz wrote:
> >currently we don't lock the shadow file when removing or storing a
> >password. by adding locking here we avoid a situation where storing
> >and/or removing a password concurrently could lead to a race
> >condition. in this scenario it is possible that a password isn't
> >persisted or a password isn't removed. we already do this for
> >the "token.shadow" file, so just use the same mechanism here.
> >
> >Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>
> Is there any reason why the store_password function does not lock the
> shadow.json file?

do you mean the `authenticate_user` function? this patch add a locking
call to the `store_password` function for me.

>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions
  2024-05-23 11:46 ` Gabriel Goller
  2024-05-23 12:06   ` Shannon Sterz
@ 2024-06-03  9:00   ` Wolfgang Bumiller
  2024-06-03  9:12     ` Gabriel Goller
  1 sibling, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2024-06-03  9:00 UTC (permalink / raw)
  To: Gabriel Goller; +Cc: Proxmox Backup Server development discussion

On Thu, May 23, 2024 at 01:46:47PM GMT, Gabriel Goller wrote:
> On 23.05.2024 13:25, Shannon Sterz wrote:
> > currently we don't lock the shadow file when removing or storing a
> > password. by adding locking here we avoid a situation where storing
> > and/or removing a password concurrently could lead to a race
> > condition. in this scenario it is possible that a password isn't
> > persisted or a password isn't removed. we already do this for
> > the "token.shadow" file, so just use the same mechanism here.
> > 
> > Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> 
> Is there any reason why the store_password function does not lock the
> shadow.json file?

I'm assuming you mean this as an alternative to having a separate file
for the locking. In this case the answer is yes: We use `repalce_file()`
to store the new version to make sure the update is atomic (and a
parallel `authenticat_user` does not read a *partial* file and fail).
When you hold a lock on a file and you *replace* it, the new file is a
separate thing with no lock on it. Old threads waiting for a lock on the
old file will continue when the lock is free, new threads waiting on the
lock on the new file will be able to acquire that simultaneously.

Iow.: you can't lock something you're *replacing*.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pbs-devel] applied: [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions
  2024-05-23 11:25 [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions Shannon Sterz
  2024-05-23 11:46 ` Gabriel Goller
@ 2024-06-03  9:02 ` Wolfgang Bumiller
  1 sibling, 0 replies; 6+ messages in thread
From: Wolfgang Bumiller @ 2024-06-03  9:02 UTC (permalink / raw)
  To: Shannon Sterz; +Cc: pbs-devel

applied, thanks

On Thu, May 23, 2024 at 01:25:59PM GMT, Shannon Sterz wrote:
> currently we don't lock the shadow file when removing or storing a
> password. by adding locking here we avoid a situation where storing
> and/or removing a password concurrently could lead to a race
> condition. in this scenario it is possible that a password isn't
> persisted or a password isn't removed. we already do this for
> the "token.shadow" file, so just use the same mechanism here.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>  src/auth.rs | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/auth.rs b/src/auth.rs
> index 21468d4b..5134e41e 100644
> --- a/src/auth.rs
> +++ b/src/auth.rs
> @@ -9,6 +9,7 @@ use std::pin::Pin;
>  use anyhow::{bail, Error};
>  use futures::Future;
>  use once_cell::sync::{Lazy, OnceCell};
> +use pbs_config::open_backup_lockfile;
>  use proxmox_router::http_bail;
>  use serde_json::json;
>  
> @@ -31,6 +32,7 @@ pub const TERM_PREFIX: &str = "PBSTERM";
>  struct PbsAuthenticator;
>  
>  pub(crate) const SHADOW_CONFIG_FILENAME: &str = configdir!("/shadow.json");
> +pub(crate) const SHADOW_LOCK_FILENAME: &str = configdir!("/shadow.json.lock");
>  
>  impl Authenticator for PbsAuthenticator {
>      fn authenticate_user<'a>(
> @@ -69,6 +71,8 @@ impl Authenticator for PbsAuthenticator {
>          _client_ip: Option<&IpAddr>,
>      ) -> Result<(), Error> {
>          let enc_password = proxmox_sys::crypt::encrypt_pw(password)?;
> +
> +        let _guard = open_backup_lockfile(SHADOW_LOCK_FILENAME, None, true);
>          let mut data = proxmox_sys::fs::file_get_json(SHADOW_CONFIG_FILENAME, Some(json!({})))?;
>          data[username.as_str()] = enc_password.into();
>  
> @@ -85,6 +89,8 @@ impl Authenticator for PbsAuthenticator {
>      }
>  
>      fn remove_password(&self, username: &UsernameRef) -> Result<(), Error> {
> +        let _guard = open_backup_lockfile(SHADOW_LOCK_FILENAME, None, true);
> +
>          let mut data = proxmox_sys::fs::file_get_json(SHADOW_CONFIG_FILENAME, Some(json!({})))?;
>          if let Some(map) = data.as_object_mut() {
>              map.remove(username.as_str());
> -- 
> 2.39.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions
  2024-06-03  9:00   ` Wolfgang Bumiller
@ 2024-06-03  9:12     ` Gabriel Goller
  0 siblings, 0 replies; 6+ messages in thread
From: Gabriel Goller @ 2024-06-03  9:12 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: Proxmox Backup Server development discussion

On 03.06.2024 11:00, Wolfgang Bumiller wrote:
>On Thu, May 23, 2024 at 01:46:47PM GMT, Gabriel Goller wrote:
>> On 23.05.2024 13:25, Shannon Sterz wrote:
>> > currently we don't lock the shadow file when removing or storing a
>> > password. by adding locking here we avoid a situation where storing
>> > and/or removing a password concurrently could lead to a race
>> > condition. in this scenario it is possible that a password isn't
>> > persisted or a password isn't removed. we already do this for
>> > the "token.shadow" file, so just use the same mechanism here.
>> >
>> > Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>>
>> Is there any reason why the store_password function does not lock the
>> shadow.json file?
>
>I'm assuming you mean this as an alternative to having a separate file
>for the locking. In this case the answer is yes: We use `repalce_file()`
>to store the new version to make sure the update is atomic (and a
>parallel `authenticat_user` does not read a *partial* file and fail).
>When you hold a lock on a file and you *replace* it, the new file is a
>separate thing with no lock on it. Old threads waiting for a lock on the
>old file will continue when the lock is free, new threads waiting on the
>lock on the new file will be able to acquire that simultaneously.
>
>Iow.: you can't lock something you're *replacing*.

Nah, this was my bad...

      impl Authenticator for PbsAuthenticator {
          fn authenticate_user<'a>(
     @@ -69,6 +71,8 @@ impl Authenticator for PbsAuthenticator {
              _client_ip: Option<&IpAddr>,
          ) -> Result<(), Error> {
              let enc_password = proxmox_sys::crypt::encrypt_pw(password)?;
     +
     +        let _guard = open_backup_lockfile(SHADOW_LOCK_FILENAME, None, true);
              let mut data = proxmox_sys::fs::file_get_json(SHADOW_CONFIG_FILENAME, Some(json!({})))?;
              data[username.as_str()] = enc_password.into();


It looks like the lines are added in `authenticate_user`, but they are
actually added in `store_password`, so it's all fine :)


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-06-03  9:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-23 11:25 [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions Shannon Sterz
2024-05-23 11:46 ` Gabriel Goller
2024-05-23 12:06   ` Shannon Sterz
2024-06-03  9:00   ` Wolfgang Bumiller
2024-06-03  9:12     ` Gabriel Goller
2024-06-03  9:02 ` [pbs-devel] applied: " Wolfgang Bumiller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal