[pbs-devel] [PATCH proxmox-backup] add datastore info api call

[pbs-devel] [PATCH proxmox-backup] add datastore info api call

[Oguz Bektas]

to be able to copy/paste easily when adding a new PBS datastore remote
in PVE

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/api2/admin/datastore.rs | 55 +++++++++++++++++++++++++++++++++++++
 src/api2/types/mod.rs       | 23 ++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 91ca3570..41059f98 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -25,6 +25,7 @@ use pxar::EntryKind;
 
 use crate::api2::types::*;
 use crate::api2::node::rrd::create_value_from_rrd;
+use crate::config::network::{self};
 use crate::backup::*;
 use crate::config::datastore;
 use crate::config::cached_user_info::CachedUserInfo;
@@ -36,6 +37,7 @@ use crate::tools::{
     AsyncChannelWriter, AsyncReaderStream, WrappedReaderStream,
 };
 
+use crate::tools::cert::CertInfo;
 use crate::config::acl::{
     PRIV_DATASTORE_AUDIT,
     PRIV_DATASTORE_MODIFY,
@@ -448,6 +450,54 @@ pub fn status(
     crate::tools::disks::disk_usage(&datastore.base_path())
 }
 
+#[api(
+    input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+        },
+    },
+    returns: {
+        type: DataStoreInfo,
+    },
+    access: {
+        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_READ, true),
+    },
+)]
+/// Get information about the datastore.
+///
+/// Provides PBS node fingerprint, address and datastore name
+pub fn info(
+    store: String,
+    _info: &ApiMethod,
+    _rpcenv: &mut dyn RpcEnvironment,
+) -> Result<DataStoreInfo, Error> {
+    let _datastore = DataStore::lookup_datastore(&store)?;
+    let cert = CertInfo::new()?;
+    let fingerprint = cert.fingerprint()?;
+
+    // get all possible interface IP addresses since there's
+    // no explicit way to tell which is needed
+    let (config, _) = network::config()?;
+    let mut address_list = Vec::new();
+    for (_ , interface) in config.interfaces.iter() {
+        if let Some(cidr) = &interface.cidr {
+            address_list.push(cidr.to_owned());
+        }
+    }
+
+    let result_item = DataStoreInfo {
+        name: store,
+        address_list,
+        fingerprint,
+    };
+
+    Ok(result_item)
+}
+
+
+
 #[api(
     input: {
         properties: {
@@ -1673,6 +1723,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
         &Router::new()
             .get(&API_METHOD_LIST_GROUPS)
     ),
+    (
+        "info",
+        &Router::new()
+            .get(&API_METHOD_INFO)
+    ),
     (
         "notes",
         &Router::new()
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index f97db557..9e61f15c 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -1070,3 +1070,26 @@ pub struct APTUpdateInfo {
     /// URL under which the package's changelog can be retrieved
     pub change_log_url: String,
 }
+
+#[api(
+    properties: {
+        "address-list": {
+            description: "List of IPs from node",
+            type: Array,
+            items: {
+                description: "CIDR",
+                type: String,
+            },
+        },
+})]
+#[derive(Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+/// Necessary information for adding a remote
+pub struct DataStoreInfo {
+    /// Name of the datastore
+    pub name: String,
+    /// Available IP addresses from the node
+    pub address_list: Vec<String>,
+    /// x509 fingerprint of the node
+    pub fingerprint: String,
+}
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup] add datastore info api call

[Fabian Grünbichler]

On October 21, 2020 4:01 pm, Oguz Bektas wrote:
> to be able to copy/paste easily when adding a new PBS datastore remote
> in PVE
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 55 +++++++++++++++++++++++++++++++++++++
>  src/api2/types/mod.rs       | 23 ++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 91ca3570..41059f98 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -25,6 +25,7 @@ use pxar::EntryKind;
>  
>  use crate::api2::types::*;
>  use crate::api2::node::rrd::create_value_from_rrd;
> +use crate::config::network::{self};
>  use crate::backup::*;
>  use crate::config::datastore;
>  use crate::config::cached_user_info::CachedUserInfo;
> @@ -36,6 +37,7 @@ use crate::tools::{
>      AsyncChannelWriter, AsyncReaderStream, WrappedReaderStream,
>  };
>  
> +use crate::tools::cert::CertInfo;
>  use crate::config::acl::{
>      PRIV_DATASTORE_AUDIT,
>      PRIV_DATASTORE_MODIFY,
> @@ -448,6 +450,54 @@ pub fn status(
>      crate::tools::disks::disk_usage(&datastore.base_path())
>  }
>  
> +#[api(
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +        },
> +    },
> +    returns: {
> +        type: DataStoreInfo,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_READ, true),

why READ and not AUDIT | BACKUP ? why partial if you only pass a single 
privilege?

> +    },
> +)]
> +/// Get information about the datastore.
> +///
> +/// Provides PBS node fingerprint, address and datastore name
> +pub fn info(
> +    store: String,
> +    _info: &ApiMethod,
> +    _rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<DataStoreInfo, Error> {
> +    let _datastore = DataStore::lookup_datastore(&store)?;
> +    let cert = CertInfo::new()?;
> +    let fingerprint = cert.fingerprint()?;
> +
> +    // get all possible interface IP addresses since there's
> +    // no explicit way to tell which is needed
> +    let (config, _) = network::config()?;
> +    let mut address_list = Vec::new();
> +    for (_ , interface) in config.interfaces.iter() {
> +        if let Some(cidr) = &interface.cidr {
> +            address_list.push(cidr.to_owned());
> +        }
> +    }

doesn't this leak information that the user would/should not have access 
to? I mean, if I can do an API call I already have some way to reach the 
PBS server and we could just default to that on the client side.. 
possibly it would make sense to declare some interface as the 
'external/public' one and return that if configured, but just returning 
all addresses of all interfaces seems a bit much..

> +
> +    let result_item = DataStoreInfo {
> +        name: store,
> +        address_list,
> +        fingerprint,
> +    };
> +
> +    Ok(result_item)
> +}
> +
> +
> +
>  #[api(
>      input: {
>          properties: {
> @@ -1673,6 +1723,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>          &Router::new()
>              .get(&API_METHOD_LIST_GROUPS)
>      ),
> +    (
> +        "info",
> +        &Router::new()
> +            .get(&API_METHOD_INFO)
> +    ),
>      (
>          "notes",
>          &Router::new()
> diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
> index f97db557..9e61f15c 100644
> --- a/src/api2/types/mod.rs
> +++ b/src/api2/types/mod.rs
> @@ -1070,3 +1070,26 @@ pub struct APTUpdateInfo {
>      /// URL under which the package's changelog can be retrieved
>      pub change_log_url: String,
>  }
> +
> +#[api(
> +    properties: {
> +        "address-list": {
> +            description: "List of IPs from node",
> +            type: Array,
> +            items: {
> +                description: "CIDR",
> +                type: String,
> +            },
> +        },
> +})]
> +#[derive(Serialize, Deserialize)]
> +#[serde(rename_all = "kebab-case")]
> +/// Necessary information for adding a remote
> +pub struct DataStoreInfo {
> +    /// Name of the datastore
> +    pub name: String,
> +    /// Available IP addresses from the node
> +    pub address_list: Vec<String>,
> +    /// x509 fingerprint of the node
> +    pub fingerprint: String,
> +}
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> 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] add datastore info api call

[Oguz Bektas]

hi,

On Thu, Oct 22, 2020 at 10:02:23AM +0200, Fabian Grünbichler wrote:
> 
> why READ and not AUDIT | BACKUP ? why partial if you only pass a single 
> privilege?

i thought the minimum privilege should be view. one might want to add a
datastore where only read access is given to them, to be able to restore
backups from it for example. imposing audit/backup privs would prevent
this, afaict

> 
> > +    },
> > +)]
> > +/// Get information about the datastore.
> > +///
> > +/// Provides PBS node fingerprint, address and datastore name
> > +pub fn info(
> > +    store: String,
> > +    _info: &ApiMethod,
> > +    _rpcenv: &mut dyn RpcEnvironment,
> > +) -> Result<DataStoreInfo, Error> {
> > +    let _datastore = DataStore::lookup_datastore(&store)?;
> > +    let cert = CertInfo::new()?;
> > +    let fingerprint = cert.fingerprint()?;
> > +
> > +    // get all possible interface IP addresses since there's
> > +    // no explicit way to tell which is needed
> > +    let (config, _) = network::config()?;
> > +    let mut address_list = Vec::new();
> > +    for (_ , interface) in config.interfaces.iter() {
> > +        if let Some(cidr) = &interface.cidr {
> > +            address_list.push(cidr.to_owned());
> > +        }
> > +    }
> 
> doesn't this leak information that the user would/should not have access 
> to? I mean, if I can do an API call I already have some way to reach the 
> PBS server and we could just default to that on the client side.. 
> possibly it would make sense to declare some interface as the 
> 'external/public' one and return that if configured, but just returning 
> all addresses of all interfaces seems a bit much..

yes, i wasn't sure how to handle this since in PVE we just take the
corosync link but here it can be any interface.

i do like the suggestion to declare an interface the "public" one.
but there could be multiple interfaces being utilized as well (like f.e.
if the server has 2 addresses on two different subnets, with different
datastores). then it would make things harder.

i'm open to different suggestions.


> 
> > +
> > +    let result_item = DataStoreInfo {
> > +        name: store,
> > +        address_list,
> > +        fingerprint,
> > +    };
> > +
> > +    Ok(result_item)
> > +}
> > +
> > +
> > +
> >  #[api(
> >      input: {
> >          properties: {
> > @@ -1673,6 +1723,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
> >          &Router::new()
> >              .get(&API_METHOD_LIST_GROUPS)
> >      ),
> > +    (
> > +        "info",
> > +        &Router::new()
> > +            .get(&API_METHOD_INFO)
> > +    ),
> >      (
> >          "notes",
> >          &Router::new()
> > diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
> > index f97db557..9e61f15c 100644
> > --- a/src/api2/types/mod.rs
> > +++ b/src/api2/types/mod.rs
> > @@ -1070,3 +1070,26 @@ pub struct APTUpdateInfo {
> >      /// URL under which the package's changelog can be retrieved
> >      pub change_log_url: String,
> >  }
> > +
> > +#[api(
> > +    properties: {
> > +        "address-list": {
> > +            description: "List of IPs from node",
> > +            type: Array,
> > +            items: {
> > +                description: "CIDR",
> > +                type: String,
> > +            },
> > +        },
> > +})]
> > +#[derive(Serialize, Deserialize)]
> > +#[serde(rename_all = "kebab-case")]
> > +/// Necessary information for adding a remote
> > +pub struct DataStoreInfo {
> > +    /// Name of the datastore
> > +    pub name: String,
> > +    /// Available IP addresses from the node
> > +    pub address_list: Vec<String>,
> > +    /// x509 fingerprint of the node
> > +    pub fingerprint: String,
> > +}
> > -- 
> > 2.20.1
> > 
> > 
> > _______________________________________________
> > 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] add datastore info api call

[Thomas Lamprecht]

On 22.10.20 11:17, Oguz Bektas wrote:
> hi,
> 
> On Thu, Oct 22, 2020 at 10:02:23AM +0200, Fabian Grünbichler wrote:
>>
>> why READ and not AUDIT | BACKUP ? why partial if you only pass a single 
>> privilege?
> 
> i thought the minimum privilege should be view. one might want to add a
> datastore where only read access is given to them, to be able to restore
> backups from it for example. imposing audit/backup privs would prevent
> this, afaict
> 
>>
>>> +    },
>>> +)]
>>> +/// Get information about the datastore.
>>> +///
>>> +/// Provides PBS node fingerprint, address and datastore name
>>> +pub fn info(
>>> +    store: String,
>>> +    _info: &ApiMethod,
>>> +    _rpcenv: &mut dyn RpcEnvironment,
>>> +) -> Result<DataStoreInfo, Error> {
>>> +    let _datastore = DataStore::lookup_datastore(&store)?;
>>> +    let cert = CertInfo::new()?;
>>> +    let fingerprint = cert.fingerprint()?;
>>> +
>>> +    // get all possible interface IP addresses since there's
>>> +    // no explicit way to tell which is needed
>>> +    let (config, _) = network::config()?;
>>> +    let mut address_list = Vec::new();
>>> +    for (_ , interface) in config.interfaces.iter() {
>>> +        if let Some(cidr) = &interface.cidr {
>>> +            address_list.push(cidr.to_owned());
>>> +        }
>>> +    }
>>
>> doesn't this leak information that the user would/should not have access 
>> to? I mean, if I can do an API call I already have some way to reach the 
>> PBS server and we could just default to that on the client side.. 
>> possibly it would make sense to declare some interface as the 
>> 'external/public' one and return that if configured, but just returning 
>> all addresses of all interfaces seems a bit much..
> 
> yes, i wasn't sure how to handle this since in PVE we just take the
> corosync link but here it can be any interface.
> 
> i do like the suggestion to declare an interface the "public" one.
> but there could be multiple interfaces being utilized as well (like f.e.
> if the server has 2 addresses on two different subnets, with different
> datastores). then it would make things harder.
> 
> i'm open to different suggestions.
> 

The gui, or the CLI client could really just add the host/address used to make
the API call and put it into the presented encoded information.

One would have the client IP through the rpcenv, and could make some decision
based on that, theoretically, but I'd prefer the "client should fill in" 
approach over this.

Re: [pbs-devel] [PATCH proxmox-backup] add datastore info api call

[Fabian Grünbichler]

On October 22, 2020 11:17 am, Oguz Bektas wrote:
> hi,
> 
> On Thu, Oct 22, 2020 at 10:02:23AM +0200, Fabian Grünbichler wrote:
>> 
>> why READ and not AUDIT | BACKUP ? why partial if you only pass a single 
>> privilege?
> 
> i thought the minimum privilege should be view. one might want to add a
> datastore where only read access is given to them, to be able to restore
> backups from it for example. imposing audit/backup privs would prevent
> this, afaict

AUDIT is the permission that determines visibility of a datastore, READ 
is the permission to read its *contents*. this api call is not related 
to reading arbitrary datastore contents, so it should not require that 
permission.

Re: [pbs-devel] [PATCH proxmox-backup] add datastore info api call

[Oguz Bektas]

On Thu, Oct 22, 2020 at 01:00:48PM +0200, Fabian Grünbichler wrote:
> On October 22, 2020 11:17 am, Oguz Bektas wrote:
> > hi,
> > 
> > On Thu, Oct 22, 2020 at 10:02:23AM +0200, Fabian Grünbichler wrote:
> >> 
> >> why READ and not AUDIT | BACKUP ? why partial if you only pass a single 
> >> privilege?
> > 
> > i thought the minimum privilege should be view. one might want to add a
> > datastore where only read access is given to them, to be able to restore
> > backups from it for example. imposing audit/backup privs would prevent
> > this, afaict
> 
> AUDIT is the permission that determines visibility of a datastore, READ 
> is the permission to read its *contents*. this api call is not related 
> to reading arbitrary datastore contents, so it should not require that 
> permission.

i see, it's clear now thanks

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