[pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions

[pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions

[Shannon Sterz]

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

Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions

[Gabriel Goller]

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

Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions

[Shannon Sterz]

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

Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions

[Wolfgang Bumiller]

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

[pbs-devel] applied: [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions

[Wolfgang Bumiller]

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

Re: [pbs-devel] [PATCH proxmox-backup] auth: add locking to `PbsAuthenticator` to avoid race conditions

[Gabriel Goller]

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