[pve-devel] fix #5254: add separate Sys.AccessNetwork privilege

[pve-devel] fix #5254: add separate Sys.AccessNetwork privilege

[Thomas Lamprecht]

Adds a new Sys.AccessNetwork privilege that can be used to guard API
endpoints that can do outgoing network requests with (some) user control
over said requests, like e.g. the "download URL to storage" one.

## Backstory:

This stems from an user request [0] w.r.t. the "download image through
and URL directly to a storage" functionality and their use case of that
through automation while wanting to adhere to the principle of least
privilege.

Because before this series the access to the required endpoints was
guarded by the more powerful Sys.Modify and Sys.Audit privilege
requirement on the / root ACL object path.
So, if anybody wants to set up an API token so that automation can
handle image downloads they'd need to give that API token very powerful
permissions to make it work.

A more specialized privilege seems warranted now, so add the
Sys.AccessNetwork one and adapt the /nodes/{node}/query-url-metadata and
the related /nodes/{node}/storage/{storage}/download-url API endpoints
for now.

## Testing:

Tested by creating a new custom role with the privileges
`Datastore.Audit,Datastore.AllocateTemplate,Sys.AccessNetwork`, then
created a user that gets a permission with above role for a specific
node and a storage and then try querying and downloading an image, with
and without this patch series applied.

## Future Work

We could this even re-use for other endpoints, like adding storages that
are accessed through the network, as that provides a (limited) side
channel too.

access-control:

Thomas Lamprecht (1):
      add Sys.AccessNetwork privilege

 src/PVE/AccessControl.pm | 1 +
 src/test/perm-test1.pl   | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

storage:

Thomas Lamprecht (1):
      fix #5254: api: allow usage of download-url with Sys.AccessNetwork

 src/PVE/API2/Storage/Status.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

manager:

Thomas Lamprecht (2):
      api: nodes: allow usage of query url metadata with Sys.AccessNetwork
      ui: storage: enable download-url button with Sys.AccessNetwork capability

 PVE/API2/Nodes.pm               | 5 ++++-
 www/manager6/storage/Browser.js | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

[pve-devel] [PATCH access-control] add Sys.AccessNetwork privilege

[Thomas Lamprecht]

We have some API endpoints that can access the network from the POV of
a Proxmox VE node, like e.g., the one for downloading a template/ISO
image directly to a PVE storage from an HTTP URL, and the matching
query-url-metadata that makes this functionality much more convenient
to use in the UI. But the downside of such calls is naturally that
they basically allow to scan the whole network via HTTP URLs, and
potentially even download some image that the user should not have
access to and adding to a VM that the user controls.

Due to that we limited the exposure of those API endpoints to
Sys.Modify on / (in addition to e.g. basic storage privs) for the
initial addition of the feature, as we were not sure about user
adoption and if a separate privilege could be justified.

Since we got a handful requests like #5254 this justification is now
met, so add a 'Sys.AccessNetwork' privilege.
That name should make it clear that having that privilege will allow
access to the network and the sys(tem) prefix should underline that
it's about the host systems network. Add it such, that it will only be
available for the most powerful of our built-in special roles, namely
the Administration one, besides naturally the all-powerful root@pam
special user.

Admins can then e.g. create new roles that include Sys.AccessNetwork
and Datastore.AllocateTemplate which can then be used for allowing
automation to download images while adhering to the Least Privilege
Principle.

Buglink: https://bugzilla.proxmox.com/show_bug.cgi?id=5254
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/AccessControl.pm | 1 +
 src/test/perm-test1.pl   | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 25fc0d9..faea70d 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1065,6 +1065,7 @@ my $privgroups = {
 	    'Sys.PowerMgmt',
 	    'Sys.Modify', # edit/change node settings
 	    'Sys.Incoming', # incoming storage/guest migrations
+	    'Sys.AccessNetwork', # for, e.g., downloading ISOs from any URL
 	],
 	admin => [
 	    'Sys.Console',
diff --git a/src/test/perm-test1.pl b/src/test/perm-test1.pl
index 27aadeb..df9fe90 100755
--- a/src/test/perm-test1.pl
+++ b/src/test/perm-test1.pl
@@ -79,7 +79,7 @@ check_permission(
     . 'Pool.Allocate,Pool.Audit,'
     . 'Realm.Allocate,Realm.AllocateUser,'
     . 'SDN.Allocate,SDN.Audit,SDN.Use,'
-    . 'Sys.Audit,Sys.Console,Sys.Incoming,Sys.Modify,Sys.PowerMgmt,Sys.Syslog,'
+    . 'Sys.AccessNetwork,Sys.Audit,Sys.Console,Sys.Incoming,Sys.Modify,Sys.PowerMgmt,Sys.Syslog,'
     . 'User.Modify,'
     . 'VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,'
     . 'VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,'
-- 
2.39.2

[pve-devel] [PATCH storage:] fix #5254: api: allow usage of download-url with Sys.AccessNetwork

[Thomas Lamprecht]

The download-url API endpoint has some implications that admins are
unaware of, namely that it basically allow to scan the whole network
via HTTP URLs, and potentially even download some image that the user
should not have access to and adding to a VM that the user controls.

That's why in addition to the Datastore.AllocateTemplate privilege on
the storage, the Sys.Modify on the whole Cluster was required to use
the API call. That design was chosen as we were not fully sure if a
separate privilege is warranted, but user feedback has shown that the
(not so big) cost of adding such a new privilege is justified.

Change the permission check to allow the combination of
Datastore.AllocateTemplate on the storage and either 'Sys.Modify' on
/, for backwards compatibility, or the newer 'Sys.AccessNetwork' on
the node that handles the download.
Using a node-specific ACL path allows admins to e.g. prepare one
specific node's firewall so that pveproxy can access only a safe set
of hosts via outgoing HTTP (not stemming from valid connection
tracking to the PVE API), and thus even further limit the privileges
of users or tools that are trusted to download images to a storage.

Buglink: https://bugzilla.proxmox.com/show_bug.cgi?id=5254
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/API2/Storage/Status.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index b2336e6..bc67b81 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -546,9 +546,15 @@ __PACKAGE__->register_method({
     description => "Download templates and ISO images by using an URL.",
     proxyto => 'node',
     permissions => {
+	description => 'Requires allocation access on the storage and as this allows one to probe'
+	    .' the (local!) host network indirectly it also requires one of Sys.Modify on / (for'
+	    .' backwards compatibility) or the newer Sys.AccessNetwork privilege on the node.',
 	check => [ 'and',
 	    ['perm', '/storage/{storage}', [ 'Datastore.AllocateTemplate' ]],
-	    ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+	    [ 'or',
+		['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+		['perm', '/nodes/{node}', [ 'Sys.AccessNetwork' ]],
+	    ],
 	],
     },
     protected => 1,
-- 
2.39.2

[pve-devel] [PATCH manager 1/2] api: nodes: allow usage of query url metadata with Sys.AccessNetwork

[Thomas Lamprecht]

This was restricted to Sys.Modify + Sys.Audit on the whole cluster to
ensure that only trusted users get access to a method that can scan
the (local) network from the POV of the Proxmox VE node, even if only
through HTTP HEAD requests.

Nowadays there's enough user interest [0] to warrant a separate access
privilege to cover such an use case, and while most of the requests
are for the download-url storage API endpoint, this method here is not
only a bit less powerful than the storage one, it's rather tied to the
latter anyway (e.g. for querying the metadata of a URL in the web UI
for name and size before downloading it to a storage).

For backwards compatibility keep the old check and add the new
privilege as alternative to fulfill the permission requirements of
that API endpoint.

[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5254

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 PVE/API2/Nodes.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 3619190d..5d9bae39 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1581,7 +1581,10 @@ __PACKAGE__->register_method({
     description => "Query metadata of an URL: file size, file name and mime type.",
     proxyto => 'node',
     permissions => {
-	check => ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+	check => ['or',
+	    ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+	    ['perm', '/nodes/{node}', [ 'Sys.AccessNetwork' ]],
+	],
     },
     parameters => {
 	additionalProperties => 0,
-- 
2.39.2

[pve-devel] [PATCH manager 2/2] ui: storage: enable download-url button with Sys.AccessNetwork capability

[Thomas Lamprecht]

Now that the new Sys.AccessNetwork privilege got added to
access-control and the storage download and manager query API
endpoints, the UI should reflect that change and allow opening the
dialogue to download an image from URL also for the users that are
allowed to do so through the new privilege.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 www/manager6/storage/Browser.js | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index 457816a6..c2ad01e9 100644
--- a/www/manager6/storage/Browser.js
+++ b/www/manager6/storage/Browser.js
@@ -51,7 +51,10 @@ Ext.define('PVE.storage.Browser', {
 	    let contents = res.content.split(',');
 
 	    let enableUpload = !!caps.storage['Datastore.AllocateTemplate'];
-	    let enableDownloadUrl = enableUpload && !!(caps.nodes['Sys.Audit'] && caps.nodes['Sys.Modify']);
+	    let enableDownloadUrl = enableUpload && (
+		!!(caps.nodes['Sys.Audit'] && caps.nodes['Sys.Modify']) || // for backward compat
+		!!caps.nodes['Sys.AccessNetwork'] // new explicit priv for querying (local) networks
+	    );
 
 	    if (contents.includes('backup')) {
 		me.items.push({
-- 
2.39.2

Re: [pve-devel] fix #5254: add separate Sys.AccessNetwork privilege

[Thomas Lamprecht]

What I forgot to mention:

- docs patch is still missing, but would just add an entry for this new priv
  in the list of existing privileges with a short description

- there aren't really hard dependencies between the patches of the different
  packages, it just won't work, so I do not think we need to actually bump
  the d/control package dependency versions for just this.

Re: [pve-devel] fix #5254: add separate Sys.AccessNetwork privilege

[Hannes Dürr]

Tested the patch by

1.

* creating a role with 
'Datastore.Audit,Datastore.AllocateTemplate,Sys.AccessNetwork'

* creating a user with permission for specific {storage} and {node}

* downloading the iso via gui

2.

* creating a role with 'Datastore.AllocateTemplate,Sys.AccessNetwork'

* creating an API Token with permission for specific {storage} and {node}

* querying image via the endpoint nodes/{node}/query-url-metadata

* downloading the image via the endpoint 
nodes/{node}/storage/{storage}/download-url

Tested-by: Hannes Duerr <h.duerr@proxmox.com>


On 2/19/24 18:14, Thomas Lamprecht wrote:
> Adds a new Sys.AccessNetwork privilege that can be used to guard API
> endpoints that can do outgoing network requests with (some) user control
> over said requests, like e.g. the "download URL to storage" one.
>
> ## Backstory:
>
> This stems from an user request [0] w.r.t. the "download image through
> and URL directly to a storage" functionality and their use case of that
> through automation while wanting to adhere to the principle of least
> privilege.
>
> Because before this series the access to the required endpoints was
> guarded by the more powerful Sys.Modify and Sys.Audit privilege
> requirement on the / root ACL object path.
> So, if anybody wants to set up an API token so that automation can
> handle image downloads they'd need to give that API token very powerful
> permissions to make it work.
>
> A more specialized privilege seems warranted now, so add the
> Sys.AccessNetwork one and adapt the /nodes/{node}/query-url-metadata and
> the related /nodes/{node}/storage/{storage}/download-url API endpoints
> for now.
>
> ## Testing:
>
> Tested by creating a new custom role with the privileges
> `Datastore.Audit,Datastore.AllocateTemplate,Sys.AccessNetwork`, then
> created a user that gets a permission with above role for a specific
> node and a storage and then try querying and downloading an image, with
> and without this patch series applied.
>
> ## Future Work
>
> We could this even re-use for other endpoints, like adding storages that
> are accessed through the network, as that provides a (limited) side
> channel too.
>
> access-control:
>
> Thomas Lamprecht (1):
>        add Sys.AccessNetwork privilege
>
>   src/PVE/AccessControl.pm | 1 +
>   src/test/perm-test1.pl   | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
>
> storage:
>
> Thomas Lamprecht (1):
>        fix #5254: api: allow usage of download-url with Sys.AccessNetwork
>
>   src/PVE/API2/Storage/Status.pm | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> manager:
>
> Thomas Lamprecht (2):
>        api: nodes: allow usage of query url metadata with Sys.AccessNetwork
>        ui: storage: enable download-url button with Sys.AccessNetwork capability
>
>   PVE/API2/Nodes.pm               | 5 ++++-
>   www/manager6/storage/Browser.js | 5 ++++-
>   2 files changed, 8 insertions(+), 2 deletions(-)
>
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] fix #5254: add separate Sys.AccessNetwork privilege

[Fabian Grünbichler]

On February 19, 2024 6:14 pm, Thomas Lamprecht wrote:
> Adds a new Sys.AccessNetwork privilege that can be used to guard API
> endpoints that can do outgoing network requests with (some) user control
> over said requests, like e.g. the "download URL to storage" one.
> 
> ## Backstory:
> 
> This stems from an user request [0] w.r.t. the "download image through
> and URL directly to a storage" functionality and their use case of that
> through automation while wanting to adhere to the principle of least
> privilege.
> 
> Because before this series the access to the required endpoints was
> guarded by the more powerful Sys.Modify and Sys.Audit privilege
> requirement on the / root ACL object path.
> So, if anybody wants to set up an API token so that automation can
> handle image downloads they'd need to give that API token very powerful
> permissions to make it work.
> 
> A more specialized privilege seems warranted now, so add the
> Sys.AccessNetwork one and adapt the /nodes/{node}/query-url-metadata and
> the related /nodes/{node}/storage/{storage}/download-url API endpoints
> for now.
> 
> ## Testing:
> 
> Tested by creating a new custom role with the privileges
> `Datastore.Audit,Datastore.AllocateTemplate,Sys.AccessNetwork`, then
> created a user that gets a permission with above role for a specific
> node and a storage and then try querying and downloading an image, with
> and without this patch series applied.
> 
> ## Future Work
> 
> We could this even re-use for other endpoints, like adding storages that
> are accessed through the network, as that provides a (limited) side
> channel too.

for the whole series:

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

this seems like a sensible addition, and is hopefully neither to
specific nor too generic for future extensions.

while the dependencies are not "hard", it might still be a good idea to
bump the min versions to ensure no weird combination is moved along the
repositories (and also, it makes it easier to tell users which version
they need, since just looking at pve-manager or pve-storage depending on
the endpoint is needed).

[pve-devel] applied-series: fix #5254: add separate Sys.AccessNetwork privilege

[Thomas Lamprecht]

Am 19/02/2024 um 18:14 schrieb Thomas Lamprecht:
> Adds a new Sys.AccessNetwork privilege that can be used to guard API
> endpoints that can do outgoing network requests with (some) user control
> over said requests, like e.g. the "download URL to storage" one.
> 
> ## Backstory:
> 
> This stems from an user request [0] w.r.t. the "download image through
> and URL directly to a storage" functionality and their use case of that
> through automation while wanting to adhere to the principle of least
> privilege.
> 
> Because before this series the access to the required endpoints was
> guarded by the more powerful Sys.Modify and Sys.Audit privilege
> requirement on the / root ACL object path.
> So, if anybody wants to set up an API token so that automation can
> handle image downloads they'd need to give that API token very powerful
> permissions to make it work.
> 
> A more specialized privilege seems warranted now, so add the
> Sys.AccessNetwork one and adapt the /nodes/{node}/query-url-metadata and
> the related /nodes/{node}/storage/{storage}/download-url API endpoints
> for now.
> 
> ## Testing:
> 
> Tested by creating a new custom role with the privileges
> `Datastore.Audit,Datastore.AllocateTemplate,Sys.AccessNetwork`, then
> created a user that gets a permission with above role for a specific
> node and a storage and then try querying and downloading an image, with
> and without this patch series applied.
> 
> ## Future Work
> 
> We could this even re-use for other endpoints, like adding storages that
> are accessed through the network, as that provides a (limited) side
> channel too.
> 
> access-control:
> 
> Thomas Lamprecht (1):
>       add Sys.AccessNetwork privilege
> 
>  src/PVE/AccessControl.pm | 1 +
>  src/test/perm-test1.pl   | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> storage:
> 
> Thomas Lamprecht (1):
>       fix #5254: api: allow usage of download-url with Sys.AccessNetwork
> 
>  src/PVE/API2/Storage/Status.pm | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> manager:
> 
> Thomas Lamprecht (2):
>       api: nodes: allow usage of query url metadata with Sys.AccessNetwork
>       ui: storage: enable download-url button with Sys.AccessNetwork capability
> 
>  PVE/API2/Nodes.pm               | 5 ++++-
>  www/manager6/storage/Browser.js | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

applied series with Hannes' T-b and Fabians' R-b, thanks for the test/review!