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

[Gabriel Goller <g.goller@proxmox.com>]

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