[pbs-devel] [PATCH proxmox-backup] chunk_store: fix problem with permission checking

[pbs-devel] [PATCH proxmox-backup] chunk_store: fix problem with permission checking

[Hannes Laimer]

Permissions are stored in the lower 9 bits (rwxrwxrwx),
so we have to mask `st_mode` with 0o777.
The datastore root dir is created with 755, the `.chunks` dir and its
contents with 750 and the `.lock` file with 644, this changes the
expected permissions accordingly.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 38a88584..29d5874a 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -576,7 +576,7 @@ impl ChunkStore {
             Ok(stat) => {
                 if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
                     || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
-                    || stat.st_mode != file_mode
+                    || stat.st_mode & 0o777 != file_mode
                 {
                     bail!(
                             "unable to open existing chunk store path {:?} - permissions or owner not correct",
@@ -598,22 +598,22 @@ impl ChunkStore {
     /// subdirectories and the lock file.
     pub fn verify_chunkstore<T: AsRef<Path>>(path: T) -> Result<(), Error> {
         // Check datastore root path perm/owner
-        ChunkStore::check_permissions(path.as_ref(), 0o700)?;
+        ChunkStore::check_permissions(path.as_ref(), 0o755)?;
 
         let chunk_dir = Self::chunk_dir(path.as_ref());
         // Check datastore .chunks path perm/owner
-        ChunkStore::check_permissions(&chunk_dir, 0o700)?;
+        ChunkStore::check_permissions(&chunk_dir, 0o750)?;
 
         // Check all .chunks subdirectories
         for i in 0..64 * 1024 {
             let mut l1path = chunk_dir.clone();
             l1path.push(format!("{:04x}", i));
-            ChunkStore::check_permissions(&l1path, 0o700)?;
+            ChunkStore::check_permissions(&l1path, 0o750)?;
         }
 
         // Check .lock file
         let lockfile_path = Self::lockfile_path(path.as_ref());
-        ChunkStore::check_permissions(lockfile_path, 0o600)?;
+        ChunkStore::check_permissions(lockfile_path, 0o644)?;
         Ok(())
     }
 }
-- 
2.39.5



_______________________________________________
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] chunk_store: fix problem with permission checking

[Gabriel Goller]

On 13.11.2024 13:40, Hannes Laimer wrote:
>Permissions are stored in the lower 9 bits (rwxrwxrwx),
>so we have to mask `st_mode` with 0o777.
>The datastore root dir is created with 755, the `.chunks` dir and its
>contents with 750 and the `.lock` file with 644, this changes the
>expected permissions accordingly.

Oops, this is my bad, I missed this.

Matching the whole st_mode exactly would be nice, but not so practical
as we would need to be generic over file/dir and symbolic link.

Also CC'ing @Wolfgang as he persuaded me to match exactly in the first
place :)

Consider:

Fixes: 6e101ff75777 ("fix #5439: allow to reuse existing datastore")
Reviewed-By: Gabriel Goller <g.goller@proxmox.com>


_______________________________________________
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] chunk_store: fix problem with permission checking

[Wolfgang Bumiller]

On Wed, Nov 13, 2024 at 03:20:54PM GMT, Gabriel Goller wrote:
> On 13.11.2024 13:40, Hannes Laimer wrote:
> > Permissions are stored in the lower 9 bits (rwxrwxrwx),
> > so we have to mask `st_mode` with 0o777.
> > The datastore root dir is created with 755, the `.chunks` dir and its
> > contents with 750 and the `.lock` file with 644, this changes the
> > expected permissions accordingly.
> 
> Oops, this is my bad, I missed this.
> 
> Matching the whole st_mode exactly would be nice, but not so practical
> as we would need to be generic over file/dir and symbolic link.
> 
> Also CC'ing @Wolfgang as he persuaded me to match exactly in the first
> place :)

Ah yes, would have had to include the mode bits...

But it seems the actual permissions were wrong as well?

(Not sure if I mentioned this, but I'm not convinced we should *fail* on
unexpected permissions, I mean, we're already changing the values in the
check now 🤷)

> Consider:
> 
> Fixes: 6e101ff75777 ("fix #5439: allow to reuse existing datastore")
> Reviewed-By: Gabriel Goller <g.goller@proxmox.com>


_______________________________________________
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] chunk_store: fix problem with permission checking

[Gabriel Goller]

On 13.11.2024 15:42, Wolfgang Bumiller wrote:
>On Wed, Nov 13, 2024 at 03:20:54PM GMT, Gabriel Goller wrote:
>> On 13.11.2024 13:40, Hannes Laimer wrote:
>> > Permissions are stored in the lower 9 bits (rwxrwxrwx),
>> > so we have to mask `st_mode` with 0o777.
>> > The datastore root dir is created with 755, the `.chunks` dir and its
>> > contents with 750 and the `.lock` file with 644, this changes the
>> > expected permissions accordingly.
>>
>> Oops, this is my bad, I missed this.
>>
>> Matching the whole st_mode exactly would be nice, but not so practical
>> as we would need to be generic over file/dir and symbolic link.
>>
>> Also CC'ing @Wolfgang as he persuaded me to match exactly in the first
>> place :)
>
>Ah yes, would have had to include the mode bits...
>
>But it seems the actual permissions were wrong as well?

Yep :)

>(Not sure if I mentioned this, but I'm not convinced we should *fail* on
>unexpected permissions, I mean, we're already changing the values in the
>check now 🤷)

Hmm I think we should fail on permissions that are too low :)
But anyway, I think it's better to be strict here – we don't want a user
to import a datastore and then something failing. (and also the user can
always change the permissions.)



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

[pbs-devel] applied-series: [PATCH proxmox-backup] chunk_store: fix problem with permission checking

[Fabian Grünbichler]

with Gabriel's trailers folded in. we can always make this more relaxed if
issues pop up.

Quoting Hannes Laimer (2024-11-13 13:40:47)
> Permissions are stored in the lower 9 bits (rwxrwxrwx),
> so we have to mask `st_mode` with 0o777.
> The datastore root dir is created with 755, the `.chunks` dir and its
> contents with 750 and the `.lock` file with 644, this changes the
> expected permissions accordingly.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-datastore/src/chunk_store.rs | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 38a88584..29d5874a 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -576,7 +576,7 @@ impl ChunkStore {
>              Ok(stat) => {
>                  if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
>                      || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
> -                    || stat.st_mode != file_mode
> +                    || stat.st_mode & 0o777 != file_mode
>                  {
>                      bail!(
>                              "unable to open existing chunk store path {:?} - permissions or owner not correct",
> @@ -598,22 +598,22 @@ impl ChunkStore {
>      /// subdirectories and the lock file.
>      pub fn verify_chunkstore<T: AsRef<Path>>(path: T) -> Result<(), Error> {
>          // Check datastore root path perm/owner
> -        ChunkStore::check_permissions(path.as_ref(), 0o700)?;
> +        ChunkStore::check_permissions(path.as_ref(), 0o755)?;
>  
>          let chunk_dir = Self::chunk_dir(path.as_ref());
>          // Check datastore .chunks path perm/owner
> -        ChunkStore::check_permissions(&chunk_dir, 0o700)?;
> +        ChunkStore::check_permissions(&chunk_dir, 0o750)?;
>  
>          // Check all .chunks subdirectories
>          for i in 0..64 * 1024 {
>              let mut l1path = chunk_dir.clone();
>              l1path.push(format!("{:04x}", i));
> -            ChunkStore::check_permissions(&l1path, 0o700)?;
> +            ChunkStore::check_permissions(&l1path, 0o750)?;
>          }
>  
>          // Check .lock file
>          let lockfile_path = Self::lockfile_path(path.as_ref());
> -        ChunkStore::check_permissions(lockfile_path, 0o600)?;
> +        ChunkStore::check_permissions(lockfile_path, 0o644)?;
>          Ok(())
>      }
>  }
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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