[pbs-devel] [PATCH proxmox-backup] fix #5439: disallow creation of datastore in root

[pbs-devel] [PATCH proxmox-backup] fix #5439: disallow creation of datastore in root

[Gabriel Goller]

Creating a datastore in root ('/') works, but afterwards gc fails (can't
traverse all directories). It might be sensible to restrict this and
disallow creation of datastores in the root directory.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/api2/config/datastore.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 6b742acb..671f07e9 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -1,7 +1,7 @@
 use std::path::PathBuf;
 
 use ::serde::{Deserialize, Serialize};
-use anyhow::Error;
+use anyhow::{bail, Error};
 use hex::FromHex;
 use serde_json::Value;
 
@@ -74,6 +74,10 @@ pub(crate) fn do_create_datastore(
 ) -> Result<(), Error> {
     let path: PathBuf = datastore.path.clone().into();
 
+    if path.parent().is_none() {
+        bail!("cannot create datastore in root path");
+    }
+
     let tuning: DatastoreTuning = serde_json::from_value(
         DatastoreTuning::API_SCHEMA
             .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
-- 
2.43.0



_______________________________________________
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] fix #5439: disallow creation of datastore in root

[Fabian Grünbichler]

On May 10, 2024 11:58 am, Gabriel Goller wrote:
> Creating a datastore in root ('/') works, but afterwards gc fails (can't
> traverse all directories). It might be sensible to restrict this and
> disallow creation of datastores in the root directory.

if we do this, we should also forbid it on the frontend side ;)

I wonder whether we shouldn't handle this in a more generic fashion
though:
- disallow path being non-empty (ignoring .zfs ?) -> `/` is not allowed
  by default
- unless a flag is set -> in case we forget to handle something, we need
  an escape hatch
- if the flag is set, check whether .chunks already exists, and if it
  does, do not recreate the chunk store

that way, we could also solve the "re-add datastore after re-install"
issue users are frequently facing..

obviously, even with that we can explicitly always forbid '/' (before or
after implementing such a mechanism), since that one is always wrong.

> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  src/api2/config/datastore.rs | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 6b742acb..671f07e9 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -1,7 +1,7 @@
>  use std::path::PathBuf;
>  
>  use ::serde::{Deserialize, Serialize};
> -use anyhow::Error;
> +use anyhow::{bail, Error};
>  use hex::FromHex;
>  use serde_json::Value;
>  
> @@ -74,6 +74,10 @@ pub(crate) fn do_create_datastore(
>  ) -> Result<(), Error> {
>      let path: PathBuf = datastore.path.clone().into();
>  
> +    if path.parent().is_none() {
> +        bail!("cannot create datastore in root path");
> +    }
> +
>      let tuning: DatastoreTuning = serde_json::from_value(
>          DatastoreTuning::API_SCHEMA
>              .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
> -- 
> 2.43.0
> 
> 
> 
> _______________________________________________
> 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] fix #5439: disallow creation of datastore in root

[Gabriel Goller]

On 16.05.2024 12:15, Fabian Grünbichler wrote:
>On May 10, 2024 11:58 am, Gabriel Goller wrote:
>> Creating a datastore in root ('/') works, but afterwards gc fails (can't
>> traverse all directories). It might be sensible to restrict this and
>> disallow creation of datastores in the root directory.
>
>if we do this, we should also forbid it on the frontend side ;)

Yes, we can do that as well.
This will probably be difficult, because AFAIK extjs doesn't support
anything like this out of the box. But creating a custom input field +
listeners to change the style should work.

>I wonder whether we shouldn't handle this in a more generic fashion
>though:
>- disallow path being non-empty (ignoring .zfs ?) -> `/` is not allowed
>  by default
>- unless a flag is set -> in case we forget to handle something, we need
>  an escape hatch
>- if the flag is set, check whether .chunks already exists, and if it
>  does, do not recreate the chunk store
>
>that way, we could also solve the "re-add datastore after re-install"
>issue users are frequently facing..

Wait, what is the issue here?

>obviously, even with that we can explicitly always forbid '/' (before or
>after implementing such a mechanism), since that one is always wrong.

TBH, I don't think there is much to gain from adding all of this.
This is still very much a niche issue IMO, as most user have a separate
disk for datastores, which makes a lot of these issues uncommon.



_______________________________________________
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] fix #5439: disallow creation of datastore in root

[Fabian Grünbichler]

On May 16, 2024 12:58 pm, Gabriel Goller wrote:
> On 16.05.2024 12:15, Fabian Grünbichler wrote:
>>On May 10, 2024 11:58 am, Gabriel Goller wrote:
>>> Creating a datastore in root ('/') works, but afterwards gc fails (can't
>>> traverse all directories). It might be sensible to restrict this and
>>> disallow creation of datastores in the root directory.
>>
>>if we do this, we should also forbid it on the frontend side ;)
> 
> Yes, we can do that as well.
> This will probably be difficult, because AFAIK extjs doesn't support
> anything like this out of the box. But creating a custom input field +
> listeners to change the style should work.

just add a validator that forbids '/', or am I missing something?

>>I wonder whether we shouldn't handle this in a more generic fashion
>>though:
>>- disallow path being non-empty (ignoring .zfs ?) -> `/` is not allowed
>>  by default
>>- unless a flag is set -> in case we forget to handle something, we need
>>  an escape hatch
>>- if the flag is set, check whether .chunks already exists, and if it
>>  does, do not recreate the chunk store
>>
>>that way, we could also solve the "re-add datastore after re-install"
>>issue users are frequently facing..
> 
> Wait, what is the issue here?

the issue is that right now, if you lose your datastore.cfg (entry) for
whatever reason (the most common would be - move the actual datastore
from one PBS to another, or re-install, or test disaster recovery), you
cannot recreate it via our API/CLI. to re-add an already existing
(on-disk) datastore you have to manually edit datastore.cfg, since
attempting to create a "new" one using the already existing path fails
since it can't create the chunk store (it's already there after all).

this happens more often than you'd think, and even if just for
stream-lining disaster recovery it would be a nice addition. of course,
the question is whether to focus on that (and just add a flag that
signifies "expect/check that the datastore is already initialized"), or
whether to have a generic "ignore that path is not empty" flag that also
covers this (that would also cover more niche use cases, but most of
those would go against our recommendations anyway, since they would
entail sharing the datastore path with other usage).

>>obviously, even with that we can explicitly always forbid '/' (before or
>>after implementing such a mechanism), since that one is always wrong.
> 
> TBH, I don't think there is much to gain from adding all of this.
> This is still very much a niche issue IMO, as most user have a separate
> disk for datastores, which makes a lot of these issues uncommon.

see above ;)


_______________________________________________
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] fix #5439: disallow creation of datastore in root

[Gabriel Goller]

On 16.05.2024 13:42, Fabian Grünbichler wrote:
>On May 16, 2024 12:58 pm, Gabriel Goller wrote:
>> On 16.05.2024 12:15, Fabian Grünbichler wrote:
>>>On May 10, 2024 11:58 am, Gabriel Goller wrote:
>>>> Creating a datastore in root ('/') works, but afterwards gc fails (can't
>>>> traverse all directories). It might be sensible to restrict this and
>>>> disallow creation of datastores in the root directory.
>>>
>>>if we do this, we should also forbid it on the frontend side ;)
>>
>> Yes, we can do that as well.
>> This will probably be difficult, because AFAIK extjs doesn't support
>> anything like this out of the box. But creating a custom input field +
>> listeners to change the style should work.
>
>just add a validator that forbids '/', or am I missing something?
>
>>>I wonder whether we shouldn't handle this in a more generic fashion
>>>though:
>>>- disallow path being non-empty (ignoring .zfs ?) -> `/` is not allowed
>>>  by default
>>>- unless a flag is set -> in case we forget to handle something, we need
>>>  an escape hatch
>>>- if the flag is set, check whether .chunks already exists, and if it
>>>  does, do not recreate the chunk store
>>>
>>>that way, we could also solve the "re-add datastore after re-install"
>>>issue users are frequently facing..
>>
>> Wait, what is the issue here?
>
>the issue is that right now, if you lose your datastore.cfg (entry) for
>whatever reason (the most common would be - move the actual datastore
>from one PBS to another, or re-install, or test disaster recovery), you
>cannot recreate it via our API/CLI. to re-add an already existing
>(on-disk) datastore you have to manually edit datastore.cfg, since
>attempting to create a "new" one using the already existing path fails
>since it can't create the chunk store (it's already there after all).
>
>this happens more often than you'd think, and even if just for
>stream-lining disaster recovery it would be a nice addition. of course,
>the question is whether to focus on that (and just add a flag that
>signifies "expect/check that the datastore is already initialized"), or
>whether to have a generic "ignore that path is not empty" flag that also
>covers this (that would also cover more niche use cases, but most of
>those would go against our recommendations anyway, since they would
>entail sharing the datastore path with other usage).

ooof, I thought we did that already (the ignore .chunks if it already
exists).

What if instead of using a flag we do this automatically? So if one
creates a datastore it automatically checks if the .chunks dir exists,
and creates or doesn't create a datastore.



_______________________________________________
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] fix #5439: disallow creation of datastore in root

[Fabian Grünbichler]

On May 16, 2024 5:35 pm, Gabriel Goller wrote:
> On 16.05.2024 13:42, Fabian Grünbichler wrote:
>>On May 16, 2024 12:58 pm, Gabriel Goller wrote:
>>> On 16.05.2024 12:15, Fabian Grünbichler wrote:
>>>>On May 10, 2024 11:58 am, Gabriel Goller wrote:
>>>>> Creating a datastore in root ('/') works, but afterwards gc fails (can't
>>>>> traverse all directories). It might be sensible to restrict this and
>>>>> disallow creation of datastores in the root directory.
>>>>
>>>>if we do this, we should also forbid it on the frontend side ;)
>>>
>>> Yes, we can do that as well.
>>> This will probably be difficult, because AFAIK extjs doesn't support
>>> anything like this out of the box. But creating a custom input field +
>>> listeners to change the style should work.
>>
>>just add a validator that forbids '/', or am I missing something?
>>
>>>>I wonder whether we shouldn't handle this in a more generic fashion
>>>>though:
>>>>- disallow path being non-empty (ignoring .zfs ?) -> `/` is not allowed
>>>>  by default
>>>>- unless a flag is set -> in case we forget to handle something, we need
>>>>  an escape hatch
>>>>- if the flag is set, check whether .chunks already exists, and if it
>>>>  does, do not recreate the chunk store
>>>>
>>>>that way, we could also solve the "re-add datastore after re-install"
>>>>issue users are frequently facing..
>>>
>>> Wait, what is the issue here?
>>
>>the issue is that right now, if you lose your datastore.cfg (entry) for
>>whatever reason (the most common would be - move the actual datastore
>>from one PBS to another, or re-install, or test disaster recovery), you
>>cannot recreate it via our API/CLI. to re-add an already existing
>>(on-disk) datastore you have to manually edit datastore.cfg, since
>>attempting to create a "new" one using the already existing path fails
>>since it can't create the chunk store (it's already there after all).
>>
>>this happens more often than you'd think, and even if just for
>>stream-lining disaster recovery it would be a nice addition. of course,
>>the question is whether to focus on that (and just add a flag that
>>signifies "expect/check that the datastore is already initialized"), or
>>whether to have a generic "ignore that path is not empty" flag that also
>>covers this (that would also cover more niche use cases, but most of
>>those would go against our recommendations anyway, since they would
>>entail sharing the datastore path with other usage).
> 
> ooof, I thought we did that already (the ignore .chunks if it already
> exists).
> 
> What if instead of using a flag we do this automatically? So if one
> creates a datastore it automatically checks if the .chunks dir exists,
> and creates or doesn't create a datastore.

I guess doing it always is also possible, provided
- we at least check that all the prefix dirs are there and have the
  right owner (else the chunk store is broken)
- we print a line that we found and re-used a pre-existing chunk store


_______________________________________________
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] fix #5439: disallow creation of datastore in root

[Gabriel Goller]

Submitted a new version!


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