[pve-devel] [PATCH-SERIE pve-access-control/pve-manager/qemu-server] check permissions on local bridge

[pve-devel] [PATCH-SERIE pve-access-control/pve-manager/qemu-server] check permissions on local bridge

[Alexandre Derumier]

add vnet/localbridge permissions management

Hi,
as we has discuted some weeks ago,
this patche serie introduce management of acl for vnets && local bridges

I have reuse current sdn permissions path, to have common paths

/sdn/vnets/<zone>/<vnet>

where the local vmbr are in a virtual "localnetwork" zone

/sdn/vnets/local/<vnet>

Vlans permissions  are also handled with
/sdn/vnets/<zone>/<vnet>/<tag>

if user have permissions on the zone, he have access to all vnets/vlan
if user have permissions on the vnet/tag, he have access to only the specific vlan.
if user have permissions on the vnet, he have access to all vlans of the vnet

I have reworked the sdn zone panel from the tree, to manage permissions
on displayed vnets.

some screenshots:

https://mutulin1.odiso.net/sdnzone-perm.png
https://mutulin1.odiso.net/localzone-perm.png


for proxmox7: (for users be able to add permissions before upgrade to pve8)
pve-access-control: patch1 (to new /vnet/vlan path)
pve-manager : patch1-2 for the new gui


changelog v2:
 - use /vnets/vlan instead /vnets.vlan
 - rework the bridge filtering when user have access only to a specific vlan
 - api2 network: always check bridge access if no filter is defined

todo:
 - add permissions on clone/restore ?



pve-access-control:

Alexandre Derumier (2):
  access control: add /sdn/vnets/<vnet>/<vlan> path
  rpcenvironnment: add check_sdn_bridge

 src/PVE/AccessControl.pm  |  1 +
 src/PVE/RPCEnvironment.pm | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)


pve-manager:

Alexandre Derumier (3):
  add vnet permissions panel
  add permissions management for "localnetwork" zone
  api2: network: check permissions for local bridges

 PVE/API2/Cluster.pm                  |  12 ++
 PVE/API2/Network.pm                  |  26 ++-
 www/manager6/Makefile                |   2 +
 www/manager6/sdn/Browser.js          |  17 +-
 www/manager6/sdn/VnetACLView.js      | 299 +++++++++++++++++++++++++++
 www/manager6/sdn/ZoneContentPanel.js |  41 ++++
 www/manager6/sdn/ZoneContentView.js  |  52 ++++-
 7 files changed, 420 insertions(+), 29 deletions(-)
 create mode 100644 www/manager6/sdn/VnetACLView.js
 create mode 100644 www/manager6/sdn/ZoneContentPanel.js

qemu-server:

Alexandre Derumier (1):
  api2: add check_bridge_access for create/update vm

 PVE/API2/Qemu.pm | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)


-- 
2.30.2

[pve-devel] [PATCH pve-access-control 1/2] access control: add /sdn/vnets/<vnet>/<vlan> path

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/AccessControl.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 89b7d90..2fdcd44 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1284,6 +1284,7 @@ sub check_path {
 	|/sdn
 	|/sdn/zones/[[:alnum:]\.\-\_]+
 	|/sdn/vnets/[[:alnum:]\.\-\_]+
+	|/sdn/vnets/[[:alnum:]\.\-\_]+/[1-9][0-9]{0,3}
 	|/storage
 	|/storage/[[:alnum:]\.\-\_]+
 	|/vms
-- 
2.30.2

[pve-devel] [PATCH v2 pve-manager 1/3] add vnet permissions panel

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 www/manager6/Makefile                |   2 +
 www/manager6/sdn/Browser.js          |  17 +-
 www/manager6/sdn/VnetACLView.js      | 299 +++++++++++++++++++++++++++
 www/manager6/sdn/ZoneContentPanel.js |  41 ++++
 www/manager6/sdn/ZoneContentView.js  |  25 ++-
 5 files changed, 366 insertions(+), 18 deletions(-)
 create mode 100644 www/manager6/sdn/VnetACLView.js
 create mode 100644 www/manager6/sdn/ZoneContentPanel.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 2b577c8e..fb9930af 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -251,10 +251,12 @@ JSSRC= 							\
 	sdn/StatusView.js				\
 	sdn/VnetEdit.js					\
 	sdn/VnetView.js					\
+	sdn/VnetACLView.js					\
 	sdn/VnetPanel.js				\
 	sdn/SubnetEdit.js				\
 	sdn/SubnetView.js				\
 	sdn/ZoneContentView.js				\
+	sdn/ZoneContentPanel.js				\
 	sdn/ZoneView.js					\
 	sdn/OptionsPanel.js				\
 	sdn/controllers/Base.js				\
diff --git a/www/manager6/sdn/Browser.js b/www/manager6/sdn/Browser.js
index 09b0c4fe..3dc5a5ad 100644
--- a/www/manager6/sdn/Browser.js
+++ b/www/manager6/sdn/Browser.js
@@ -25,14 +25,15 @@ Ext.define('PVE.sdn.Browser', {
 
 	const caps = Ext.state.Manager.get('GuiCap');
 
-	if (caps.sdn['SDN.Audit']) {
-	    me.items.push({
-		xtype: 'pveSDNZoneContentView',
-		title: gettext('Content'),
-		iconCls: 'fa fa-th',
-		itemId: 'content',
-	    });
-	}
+	me.items.push({
+	    nodename: nodename,
+	    zone: sdnId,
+	    xtype: 'pveSDNZoneContentPanel',
+	    title: gettext('Content'),
+	    iconCls: 'fa fa-th',
+	    itemId: 'content',
+	});
+
 	if (caps.sdn['Permissions.Modify']) {
 	    me.items.push({
 		xtype: 'pveACLView',
diff --git a/www/manager6/sdn/VnetACLView.js b/www/manager6/sdn/VnetACLView.js
new file mode 100644
index 00000000..8e7f35c1
--- /dev/null
+++ b/www/manager6/sdn/VnetACLView.js
@@ -0,0 +1,299 @@
+Ext.define('PVE.sdn.VnetACLAdd', {
+    extend: 'Proxmox.window.Edit',
+    alias: ['widget.pveSDNVnetACLAdd'],
+
+    url: '/access/acl',
+    method: 'PUT',
+    isAdd: true,
+    isCreate: true,
+
+    width: 400,
+    initComponent: function() {
+        let me = this;
+
+	let items = [
+	    {
+		xtype: 'hiddenfield',
+		name: 'path',
+		value: me.path,
+		allowBlank: false,
+		fieldLabel: gettext('Path'),
+	    },
+	];
+
+	if (me.aclType === 'group') {
+	    me.subject = gettext("Group Permission");
+	    items.push({
+		xtype: 'pveGroupSelector',
+		name: 'groups',
+		fieldLabel: gettext('Group'),
+	    });
+	} else if (me.aclType === 'user') {
+	    me.subject = gettext("User Permission");
+	    items.push({
+		xtype: 'pmxUserSelector',
+		name: 'users',
+		fieldLabel: gettext('User'),
+	    });
+	} else if (me.aclType === 'token') {
+	    me.subject = gettext("API Token Permission");
+	    items.push({
+		xtype: 'pveTokenSelector',
+		name: 'tokens',
+		fieldLabel: gettext('API Token'),
+	    });
+	} else {
+	    throw "unknown ACL type";
+	}
+
+	items.push({
+	    xtype: 'pmxRoleSelector',
+	    name: 'roles',
+	    value: 'NoAccess',
+	    fieldLabel: gettext('Role'),
+	});
+
+	items.push({
+	    xtype: 'proxmoxintegerfield',
+	    name: 'vlan',
+	    minValue: 1,
+	    maxValue: 4096,
+            allowBlank: true,
+	    fieldLabel: 'Vlan',
+	    emptyText: gettext('All'),
+	});
+
+	if (!me.path) {
+	    items.push({
+		xtype: 'proxmoxcheckbox',
+		name: 'propagate',
+		checked: true,
+		uncheckedValue: 0,
+		fieldLabel: gettext('Propagate'),
+	    });
+	}
+
+	let ipanel = Ext.create('Proxmox.panel.InputPanel', {
+	    items: items,
+	    onlineHelp: 'pveum_permission_management',
+	    onGetValues: function(values) {
+		if (values.vlan) {
+		    values.path = values.path + "/" + values.vlan;
+		    delete values.vlan;
+		}
+		return values;
+	    },
+	});
+
+	Ext.apply(me, {
+	    items: [ipanel],
+	});
+
+	me.callParent();
+    },
+});
+
+Ext.define('PVE.sdn.VnetACLView', {
+    extend: 'Ext.grid.GridPanel',
+
+    alias: ['widget.pveSDNVnetACLView'],
+
+    onlineHelp: 'chapter_user_management',
+
+    stateful: true,
+    stateId: 'grid-acls',
+
+    // use fixed path
+    path: undefined,
+
+    setPath: function(path) {
+        let me = this;
+
+        me.path = path;
+
+        if (path === undefined) {
+	    me.down('#groupmenu').setDisabled(true);
+	    me.down('#usermenu').setDisabled(true);
+	    me.down('#tokenmenu').setDisabled(true);
+        } else {
+	    me.down('#groupmenu').setDisabled(false);
+	    me.down('#usermenu').setDisabled(false);
+	    me.down('#tokenmenu').setDisabled(false);
+            me.store.load();
+        }
+    },
+    initComponent: function() {
+	let me = this;
+
+	let store = Ext.create('Ext.data.Store', {
+	    model: 'pve-acl',
+	    proxy: {
+                type: 'proxmox',
+		url: "/api2/json/access/acl",
+	    },
+	    sorters: {
+		property: 'path',
+		direction: 'ASC',
+	    },
+	});
+
+	store.addFilter(Ext.create('Ext.util.Filter', {
+	    filterFn: item => item.data.path.replace(/(\/sdn\/vnets\/)(.*)\/[0-9]*$/, '$1$2') === me.path,
+	}));
+
+	let render_ugid = function(ugid, metaData, record) {
+	    if (record.data.type === 'group') {
+		return '@' + ugid;
+	    }
+
+	    return Ext.String.htmlEncode(ugid);
+	};
+
+	let render_vlan = function(path, metaData, record) {
+	    let vlan = 'any';
+	    const match = path.match(/(\/sdn\/vnets\/)(.*)\/([0-9]*)$/);
+	    if (match) {
+		vlan = match[3];
+	    }
+
+	    return Ext.String.htmlEncode(vlan);
+	};
+
+	let columns = [
+	    {
+		header: gettext('User') + '/' + gettext('Group') + '/' + gettext('API Token'),
+		flex: 1,
+		sortable: true,
+		renderer: render_ugid,
+		dataIndex: 'ugid',
+	    },
+	    {
+		header: gettext('Role'),
+		flex: 1,
+		sortable: true,
+		dataIndex: 'roleid',
+	    },
+	    {
+		header: gettext('Vlan'),
+		flex: 1,
+		sortable: true,
+		renderer: render_vlan,
+		dataIndex: 'path',
+	    },
+	];
+
+
+	let sm = Ext.create('Ext.selection.RowModel', {});
+
+	let remove_btn = new Proxmox.button.Button({
+	    text: gettext('Remove'),
+	    disabled: true,
+	    selModel: sm,
+	    confirmMsg: gettext('Are you sure you want to remove this entry'),
+	    handler: function(btn, event, rec) {
+		var params = {
+		    'delete': 1,
+		    path: rec.data.path,
+		    roles: rec.data.roleid,
+		};
+		if (rec.data.type === 'group') {
+		    params.groups = rec.data.ugid;
+		} else if (rec.data.type === 'user') {
+		    params.users = rec.data.ugid;
+		} else if (rec.data.type === 'token') {
+		    params.tokens = rec.data.ugid;
+		} else {
+		    throw 'unknown data type';
+		}
+
+		Proxmox.Utils.API2Request({
+		    url: '/access/acl',
+		    params: params,
+		    method: 'PUT',
+		    waitMsgTarget: me,
+		    callback: () => store.load(),
+		    failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+		});
+	    },
+	});
+
+	Proxmox.Utils.monStoreErrors(me, store);
+
+	Ext.apply(me, {
+	    store: store,
+	    selModel: sm,
+	    tbar: [
+		{
+		    text: gettext('Add'),
+		    menu: {
+			xtype: 'menu',
+			items: [
+			    {
+				text: gettext('Group Permission'),
+				disabled: !me.path,
+				itemId: 'groupmenu',
+				iconCls: 'fa fa-fw fa-group',
+				handler: function() {
+				    var win = Ext.create('PVE.sdn.VnetACLAdd', {
+					aclType: 'group',
+					path: me.path,
+				    });
+				    win.on('destroy', () => store.load());
+				    win.show();
+				},
+			    },
+			    {
+				text: gettext('User Permission'),
+				disabled: !me.path,
+				itemId: 'usermenu',
+				iconCls: 'fa fa-fw fa-user',
+				handler: function() {
+				    var win = Ext.create('PVE.sdn.VnetACLAdd', {
+					aclType: 'user',
+					path: me.path,
+				    });
+				    win.on('destroy', () => store.load());
+				    win.show();
+				},
+			    },
+			    {
+				text: gettext('API Token Permission'),
+				disabled: !me.path,
+				itemId: 'tokenmenu',
+				iconCls: 'fa fa-fw fa-user-o',
+				handler: function() {
+				    let win = Ext.create('PVE.sdn.VnetACLAdd', {
+					aclType: 'token',
+					path: me.path,
+				    });
+				    win.on('destroy', () => store.load());
+				    win.show();
+				},
+			    },
+			],
+		    },
+		},
+		remove_btn,
+	    ],
+	    viewConfig: {
+		trackOver: false,
+	    },
+	    columns: columns,
+	    listeners: {
+	    },
+	});
+
+	me.callParent();
+    },
+}, function() {
+    Ext.define('pve-acl-vnet', {
+	extend: 'Ext.data.Model',
+	fields: [
+	    'path', 'type', 'ugid', 'roleid',
+	    {
+		name: 'propagate',
+		type: 'boolean',
+	    },
+	],
+    });
+});
diff --git a/www/manager6/sdn/ZoneContentPanel.js b/www/manager6/sdn/ZoneContentPanel.js
new file mode 100644
index 00000000..5bb081bb
--- /dev/null
+++ b/www/manager6/sdn/ZoneContentPanel.js
@@ -0,0 +1,41 @@
+Ext.define('PVE.sdn.ZoneContentPanel', {
+    extend: 'Ext.panel.Panel',
+    alias: 'widget.pveSDNZoneContentPanel',
+
+    title: 'Vnet',
+
+    onlineHelp: 'pvesdn_config_vnet',
+
+    initComponent: function() {
+	var me = this;
+
+	var permissions_panel = Ext.createWidget('pveSDNVnetACLView', {
+	    title: gettext('Vnet Permissions'),
+	    region: 'center',
+	    border: false,
+	});
+
+	var vnetview_panel = Ext.createWidget('pveSDNZoneContentView', {
+	    title: 'Vnets',
+	    region: 'west',
+	    permissions_panel: permissions_panel,
+	    nodename: me.nodename,
+	    zone: me.zone,
+	    width: '50%',
+	    border: false,
+	    split: true,
+	});
+
+	Ext.apply(me, {
+	    layout: 'border',
+	    items: [vnetview_panel, permissions_panel],
+	    listeners: {
+		show: function() {
+		    permissions_panel.fireEvent('show', permissions_panel);
+		},
+	    },
+	});
+
+	me.callParent();
+    },
+});
diff --git a/www/manager6/sdn/ZoneContentView.js b/www/manager6/sdn/ZoneContentView.js
index 1ea65450..08fa9d81 100644
--- a/www/manager6/sdn/ZoneContentView.js
+++ b/www/manager6/sdn/ZoneContentView.js
@@ -17,17 +17,15 @@ Ext.define('PVE.sdn.ZoneContentView', {
     initComponent: function() {
 	var me = this;
 
-	var nodename = me.pveSelNode.data.node;
-	if (!nodename) {
+	if (!me.nodename) {
 	    throw "no node name specified";
 	}
 
-	var zone = me.pveSelNode.data.sdn;
-	if (!zone) {
+	if (!me.zone) {
 	    throw "no zone ID specified";
 	}
 
-	var baseurl = "/nodes/" + nodename + "/sdn/zones/" + zone + "/content";
+	var baseurl = "/nodes/" + me.nodename + "/sdn/zones/" + me.zone + "/content";
 	var store = Ext.create('Ext.data.Store', {
 	    model: 'pve-sdnzone-content',
 	    groupField: 'content',
@@ -48,7 +46,6 @@ Ext.define('PVE.sdn.ZoneContentView', {
 	};
 
 	Proxmox.Utils.monStoreErrors(me, store);
-
 	Ext.apply(me, {
 	    store: store,
 	    selModel: sm,
@@ -79,11 +76,19 @@ Ext.define('PVE.sdn.ZoneContentView', {
 		    dataIndex: 'statusmsg',
 		},
 	    ],
-	    listeners: {
-		activate: reload,
-	    },
+            listeners: {
+                activate: reload,
+                show: reload,
+                select: function(_sm, rec) {
+                    let path = `/sdn/vnets/${rec.data.vnet}`;
+                    me.permissions_panel.setPath(path);
+                },
+                deselect: function() {
+                    me.permissions_panel.setPath(undefined);
+                },
+            },
 	});
-
+	store.load();
 	me.callParent();
     },
 }, function() {
-- 
2.30.2

[pve-devel] [PATCH v2 qemu-server 1/1] api2: add check_bridge_access for create/update vm

[Alexandre Derumier]

test first if user have access to the full zone (any bridge/vlan)
if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Qemu.pm | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 587bb22..4de7b32 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -46,6 +46,12 @@ use PVE::SSHInfo;
 use PVE::Replication;
 use PVE::StorageTunnel;
 
+my $have_sdn;
+eval {
+    require PVE::Network::SDN;
+    $have_sdn = 1;
+};
+
 BEGIN {
     if (!$ENV{PVE_GENERATING_DOCS}) {
 	require PVE::HA::Env::PVE2;
@@ -601,6 +607,34 @@ my $check_vm_create_usb_perm = sub {
     return 1;
 };
 
+my $check_bridge_access = sub {
+    my ($rpcenv, $authuser, $param) = @_;
+
+    return 1 if $authuser eq 'root@pam';
+
+    foreach my $opt (keys %{$param}) {
+	next if $opt !~ m/^net\d+$/;
+	my $net = PVE::QemuServer::parse_net($param->{$opt});
+	my $bridge = $net->{bridge};
+	my $tag = $net->{tag};
+	my $zone = 'local';
+
+	if ($have_sdn) {
+	    my $vnet_cfg = PVE::Network::SDN::Vnets::config();
+	    if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $bridge, 1))) {
+		$zone = $vnet->{zone};
+	    }
+	}
+	# test first if user have access to the full zone (any bridge/vlan)
+	return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1);
+	# if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
+	return 1 if $tag && $rpcenv->check_any($authuser, "/sdn/vnets/$bridge/$tag", ['SDN.Audit', 'SDN.Allocate'], 1);
+	# if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)
+	$rpcenv->check_any($authuser, "/sdn/vnets/$bridge", ['SDN.Audit', 'SDN.Allocate']);
+    }
+    return 1;
+};
+
 my $check_vm_modify_config_perm = sub {
     my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
 
@@ -878,7 +912,7 @@ __PACKAGE__->register_method({
 
 	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
 	    &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
-
+	    &$check_bridge_access($rpcenv, $authuser, $param);
 	    &$check_cpu_model_access($rpcenv, $authuser, $param);
 
 	    $check_drive_param->($param, $storecfg);
@@ -1578,6 +1612,8 @@ my $update_vm_api  = sub {
 
     &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param);
 
+    &$check_bridge_access($rpcenv, $authuser, $param);
+
     my $updatefn =  sub {
 
 	my $conf = PVE::QemuConfig->load_config($vmid);
-- 
2.30.2

Re: [pve-devel] [PATCH v2 qemu-server 1/1] api2: add check_bridge_access for create/update vm

[Thomas Lamprecht]

Am 05/06/2023 um 01:37 schrieb Alexandre Derumier:
> test first if user have access to the full zone (any bridge/vlan)
> if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
> if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Qemu.pm | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..4de7b32 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -46,6 +46,12 @@ use PVE::SSHInfo;
>  use PVE::Replication;
>  use PVE::StorageTunnel;
>  
> +my $have_sdn;
> +eval {
> +    require PVE::Network::SDN;
> +    $have_sdn = 1;
> +};
> +
>  BEGIN {
>      if (!$ENV{PVE_GENERATING_DOCS}) {
>  	require PVE::HA::Env::PVE2;
> @@ -601,6 +607,34 @@ my $check_vm_create_usb_perm = sub {
>      return 1;
>  };
>  
> +my $check_bridge_access = sub {
> +    my ($rpcenv, $authuser, $param) = @_;
> +
> +    return 1 if $authuser eq 'root@pam';
> +
> +    foreach my $opt (keys %{$param}) {
> +	next if $opt !~ m/^net\d+$/;
> +	my $net = PVE::QemuServer::parse_net($param->{$opt});
> +	my $bridge = $net->{bridge};
> +	my $tag = $net->{tag};

should below move to a method in pve-guest-common, taking $bridge (or name it already $vnet) and $tag
as additional parameter, and then be also used by container?

> +	my $zone = 'local';
> +
> +	if ($have_sdn) {
> +	    my $vnet_cfg = PVE::Network::SDN::Vnets::config();
> +	    if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $bridge, 1))) {
> +		$zone = $vnet->{zone};
> +	    }
> +	}
> +	# test first if user have access to the full zone (any bridge/vlan)
> +	return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1);
> +	# if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
> +	return 1 if $tag && $rpcenv->check_any($authuser, "/sdn/vnets/$bridge/$tag", ['SDN.Audit', 'SDN.Allocate'], 1);
> +	# if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)
> +	$rpcenv->check_any($authuser, "/sdn/vnets/$bridge", ['SDN.Audit', 'SDN.Allocate']);
> +    }
> +    return 1;
> +};
> +
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> @@ -878,7 +912,7 @@ __PACKAGE__->register_method({
>  
>  	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
>  	    &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
> -
> +	    &$check_bridge_access($rpcenv, $authuser, $param);
>  	    &$check_cpu_model_access($rpcenv, $authuser, $param);
>  
>  	    $check_drive_param->($param, $storecfg);
> @@ -1578,6 +1612,8 @@ my $update_vm_api  = sub {
>  
>      &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param);
>  
> +    &$check_bridge_access($rpcenv, $authuser, $param);
> +
>      my $updatefn =  sub {
>  
>  	my $conf = PVE::QemuConfig->load_config($vmid);

Re: [pve-devel] [PATCH v2 qemu-server 1/1] api2: add check_bridge_access for create/update vm

[DERUMIER, Alexandre]

> >  
> > +my $check_bridge_access = sub {
> > +    my ($rpcenv, $authuser, $param) = @_;
> > +
> > +    return 1 if $authuser eq 'root@pam';
> > +
> > +    foreach my $opt (keys %{$param}) {
> > +       next if $opt !~ m/^net\d+$/;
> > +       my $net = PVE::QemuServer::parse_net($param->{$opt});
> > +       my $bridge = $net->{bridge};
> > +       my $tag = $net->{tag};
> 
> should below move to a method in pve-guest-common, taking $bridge (or
> name it already $vnet) and $tag
> as additional parameter, and then be also used by container?

yes indeed! (I'll try to do patch for lxc today).

Re: [pve-devel] [PATCH v2 qemu-server 1/1] api2: add check_bridge_access for create/update vm

[Fabian Grünbichler]

On June 5, 2023 1:37 am, Alexandre Derumier wrote:
> test first if user have access to the full zone (any bridge/vlan)
> if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
> if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Qemu.pm | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..4de7b32 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -46,6 +46,12 @@ use PVE::SSHInfo;
>  use PVE::Replication;
>  use PVE::StorageTunnel;
>  
> +my $have_sdn;
> +eval {
> +    require PVE::Network::SDN;
> +    $have_sdn = 1;
> +};
> +
>  BEGIN {
>      if (!$ENV{PVE_GENERATING_DOCS}) {
>  	require PVE::HA::Env::PVE2;
> @@ -601,6 +607,34 @@ my $check_vm_create_usb_perm = sub {
>      return 1;
>  };
>  
> +my $check_bridge_access = sub {
> +    my ($rpcenv, $authuser, $param) = @_;
> +
> +    return 1 if $authuser eq 'root@pam';
> +
> +    foreach my $opt (keys %{$param}) {
> +	next if $opt !~ m/^net\d+$/;
> +	my $net = PVE::QemuServer::parse_net($param->{$opt});
> +	my $bridge = $net->{bridge};
> +	my $tag = $net->{tag};
> +	my $zone = 'local';
> +
> +	if ($have_sdn) {
> +	    my $vnet_cfg = PVE::Network::SDN::Vnets::config();
> +	    if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $bridge, 1))) {
> +		$zone = $vnet->{zone};
> +	    }
> +	}
> +	# test first if user have access to the full zone (any bridge/vlan)
> +	return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1);
> +	# if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
> +	return 1 if $tag && $rpcenv->check_any($authuser, "/sdn/vnets/$bridge/$tag", ['SDN.Audit', 'SDN.Allocate'], 1);

this one IMHO should be

$rpcenv->check($authuser, "/sdn/vnets/$bridge/$tag", ['SDN.Use']) if $tag;

see comment in cover letter and below.

> +	# if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)
> +	$rpcenv->check_any($authuser, "/sdn/vnets/$bridge", ['SDN.Audit', 'SDN.Allocate']);

trunks also allow (sub)ranges, in that case we could check for those
tags? or do we really need the full bridge?

these checks here should check for (a not yet existing) 'SDN.Use'
privilege, so that we can differentiate between:

- Audit: see the config, but neither change nor use its entries
- Use: use the entries
- Allocate: modify the entries

similar to Dominik's series for PCI/USB (where there is 'Modify' to
manage the entries, and 'Use' to reference them in guest configs).

I agree with Thomas that it likely makes sense to have all of that after
the parse_net call in a helper in pve-guest-common, like:

sub check_vnet_access {
  my ($rpcenv, $authuser, $vnet, $tag) = @_;

  ...
}

for code reuse with pve-container.

> +    }
> +    return 1;
> +};
> +
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> @@ -878,7 +912,7 @@ __PACKAGE__->register_method({
>  
>  	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
>  	    &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
> -
> +	    &$check_bridge_access($rpcenv, $authuser, $param);
>  	    &$check_cpu_model_access($rpcenv, $authuser, $param);
>  
>  	    $check_drive_param->($param, $storecfg);
> @@ -1578,6 +1612,8 @@ my $update_vm_api  = sub {
>  
>      &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param);
>  
> +    &$check_bridge_access($rpcenv, $authuser, $param);
> +
>      my $updatefn =  sub {
>  
>  	my $conf = PVE::QemuConfig->load_config($vmid);
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH v2 pve-manager 2/3] add permissions management for "localnetwork" zone

[Alexandre Derumier]

add a default virtual zone called 'localnetwork' in the ressource tree,
and handle permissions like a true sdn zone

(no conflict with true sdn zone is possible, as they have 8 characters max)

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Cluster.pm                 | 12 ++++++++++++
 PVE/API2/Network.pm                 |  5 +++--
 www/manager6/sdn/ZoneContentView.js | 27 ++++++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 2e942368..ce0db5f8 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -474,6 +474,18 @@ __PACKAGE__->register_method({
 	    }
 	}
 
+	#add default "localnetwork" zone
+	foreach my $node (@$nodelist) {
+	    my $local_sdn = {
+		id => "sdn/$node/localnetwork",
+		sdn => 'localnetwork',
+		node => $node,
+		type => 'sdn',
+		status => 'ok',
+	    };
+	    push @$res, $local_sdn;
+	}
+
 	if ($have_sdn) {
 	    if (!$param->{type} || $param->{type} eq 'sdn') {
 
diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index a43579fa..b3faba1a 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -209,7 +209,7 @@ __PACKAGE__->register_method({
 	    type => {
 		description => "Only list specific interface types.",
 		type => 'string',
-		enum => [ @$network_type_enum, 'any_bridge' ],
+		enum => [ @$network_type_enum, 'any_bridge', 'any_local_bridge' ],
 		optional => 1,
 	    },
 	},
@@ -254,7 +254,8 @@ __PACKAGE__->register_method({
 
 	    for my $k (sort keys $ifaces->%*) {
 		my $type = $ifaces->{$k}->{type};
-		my $match = $tfilter eq $type || ($tfilter eq 'any_bridge' && ($type eq 'bridge' || $type eq 'OVSBridge'));
+		my $match = $tfilter eq $type || ($tfilter =~ /^any(_local)?_bridge$/ && ($type eq 'bridge' || $type eq 'OVSBridge'));
+
 		delete $ifaces->{$k} if !($match && $can_access_vnet->($k));
 	    }
 
diff --git a/www/manager6/sdn/ZoneContentView.js b/www/manager6/sdn/ZoneContentView.js
index 08fa9d81..0eaa742a 100644
--- a/www/manager6/sdn/ZoneContentView.js
+++ b/www/manager6/sdn/ZoneContentView.js
@@ -26,6 +26,9 @@ Ext.define('PVE.sdn.ZoneContentView', {
 	}
 
 	var baseurl = "/nodes/" + me.nodename + "/sdn/zones/" + me.zone + "/content";
+	if (me.zone === 'localnetwork') {
+	    baseurl = "/nodes/" + me.nodename + "/network?type=any_local_bridge";
+	}
 	var store = Ext.create('Ext.data.Store', {
 	    model: 'pve-sdnzone-content',
 	    groupField: 'content',
@@ -95,7 +98,29 @@ Ext.define('PVE.sdn.ZoneContentView', {
     Ext.define('pve-sdnzone-content', {
 	extend: 'Ext.data.Model',
 	fields: [
-	    'vnet', 'status', 'statusmsg',
+	    {
+		name: 'iface',
+		convert: function(value, record) {
+		    //map local vmbr to vnet
+		    if (record.data.iface) {
+			record.data.vnet = record.data.iface;
+		    }
+		    return value;
+		},
+	    },
+	    {
+		name: 'comments',
+		convert: function(value, record) {
+		    //map local vmbr comments to vnet alias
+		    if (record.data.comments) {
+			record.data.alias = record.data.comments;
+		    }
+		    return value;
+		},
+	    },
+	    'vnet',
+	    'status',
+	    'statusmsg',
 	    {
 		name: 'text',
 		convert: function(value, record) {
-- 
2.30.2

[pve-devel] [PATCH pve-access-control 2/2] rpcenvironnment: add check_sdn_bridge

[Alexandre Derumier]

check if user have access to 1 vlan of the bridge
or the bridge itself

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/RPCEnvironment.pm | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index 8586938..fb010cc 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -324,6 +324,23 @@ sub check_full {
     }
 }
 
+sub check_sdn_bridge {
+    my ($self, $username, $path, $privs, $noerr) = @_;
+
+    my $cfg = $self->{user_cfg};
+    my $bridge_acl = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, $path);
+    if ($bridge_acl) {
+	my $vlans = $bridge_acl->{children};
+	for my $vlan (keys %$vlans) {
+	    my $vlanpath = "$path/$vlan";
+	    return 1 if $self->check_any($username, $vlanpath, $privs, $noerr);
+	}
+	# check propagate on bridge itself
+	return 1 if $self->check_any($username, $path, $privs, $noerr);
+    }
+    return;
+}
+
 sub check_user_enabled {
     my ($self, $user, $noerr) = @_;
 
-- 
2.30.2

Re: [pve-devel] [PATCH pve-access-control 2/2] rpcenvironnment: add check_sdn_bridge

[Fabian Grünbichler]

On June 5, 2023 1:37 am, Alexandre Derumier wrote:
> check if user have access to 1 vlan of the bridge
> or the bridge itself
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  src/PVE/RPCEnvironment.pm | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 8586938..fb010cc 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -324,6 +324,23 @@ sub check_full {
>      }
>  }
>  
> +sub check_sdn_bridge {
> +    my ($self, $username, $path, $privs, $noerr) = @_;

instead of $path, passing in just the bridge ID would also work, and
maybe be a nicer interface..

> +
> +    my $cfg = $self->{user_cfg};
> +    my $bridge_acl = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, $path);
> +    if ($bridge_acl) {
> +	my $vlans = $bridge_acl->{children};
> +	for my $vlan (keys %$vlans) {
> +	    my $vlanpath = "$path/$vlan";
> +	    return 1 if $self->check_any($username, $vlanpath, $privs, $noerr);
> +	}
> +	# check propagate on bridge itself
> +	return 1 if $self->check_any($username, $path, $privs, $noerr);

this doesn't actually check propagation though? for that you could
either:
- use $self->permissions (it returns the propagate bit)
- query a non-existing vlan child path with check_any

> +    }
> +    return;
> +}
> +
>  sub check_user_enabled {
>      my ($self, $user, $noerr) = @_;
>  
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH pve-access-control 2/2] rpcenvironnment: add check_sdn_bridge

[DERUMIER, Alexandre]

> > +       # check propagate on bridge itself
> > +       return 1 if $self->check_any($username, $path, $privs,
> > $noerr);
> 
> this doesn't actually check propagation though? for that you could
> either:
> - use $self->permissions (it returns the propagate bit)
> - query a non-existing vlan child path with check_any
> 
> 

do we really need to check propagation ?


Here, we want to check if user have permission to the bridge,

if user have an acl on a vlan of the bridge

or 

if user have access to the bridge (propagate or not).

for example, if I check with a dummy vlanid ,/sdn/zones/myzone/vnet1/0,

It'll be ok if user have propagate on vnet1,  but not if user
don't have propagate

Re: [pve-devel] [PATCH pve-access-control 2/2] rpcenvironnment: add check_sdn_bridge

[Fabian Grünbichler]

On June 6, 2023 2:15 pm, DERUMIER, Alexandre wrote:
>> > +       # check propagate on bridge itself
>> > +       return 1 if $self->check_any($username, $path, $privs,
>> > $noerr);
>> 
>> this doesn't actually check propagation though? for that you could
>> either:
>> - use $self->permissions (it returns the propagate bit)
>> - query a non-existing vlan child path with check_any
>> 
>> 
> 
> do we really need to check propagation ?
> 
> Here, we want to check if user have permission to the bridge,
> 
> if user have an acl on a vlan of the bridge
> 
> or 
> 
> if user have access to the bridge (propagate or not).
> 
> for example, if I check with a dummy vlanid ,/sdn/zones/myzone/vnet1/0,
> 
> It'll be ok if user have propagate on vnet1,  but not if user
> don't have propagate

you are right, we don't need to check for propagation here. so basically
we have two parts - maybe those could be added as a comment, and another
higher-level one for the whole helper to make it clear what it actually
checks:

# checks whether user has $privs on the bridge/vnet in any fashion
sub check_sdn_bridge {
..
# check explicit VLAN tag ACLs
..
# check bridge/vnet itself
..
}

and then we could also turn the order around, and check the bridge
first as a fast path that does less work?

[pve-devel] [PATCH v2 pve-manager 3/3] api2: network: check permissions for local bridges

[Alexandre Derumier]

always check permissions, also when not filtered

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Network.pm | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index b3faba1a..55a37c44 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -240,23 +240,17 @@ __PACKAGE__->register_method({
 
 	if (my $tfilter = $param->{type}) {
 	    my $vnets;
-	    my $vnet_cfg;
-	    my $can_access_vnet = sub { # only matters for the $have_sdn case, checked implict
-		return 1 if $authuser eq 'root@pam' || !defined($vnets);
-		return 1 if !defined(PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $_[0], 1)); # not a vnet
-		$rpcenv->check_any($authuser, "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1)
-	    };
 
 	    if ($have_sdn && $param->{type} eq 'any_bridge') {
 		$vnets = PVE::Network::SDN::get_local_vnets(); # returns already access-filtered
-		$vnet_cfg = PVE::Network::SDN::Vnets::config();
 	    }
 
 	    for my $k (sort keys $ifaces->%*) {
 		my $type = $ifaces->{$k}->{type};
-		my $match = $tfilter eq $type || ($tfilter =~ /^any(_local)?_bridge$/ && ($type eq 'bridge' || $type eq 'OVSBridge'));
-
-		delete $ifaces->{$k} if !($match && $can_access_vnet->($k));
+		my $match = ($param->{type} eq $type) || (
+		    ($param->{type} =~ /^any(_local)?_bridge$/) &&
+		    ($type eq 'bridge' || $type eq 'OVSBridge'));
+		delete $ifaces->{$k} if !$match;
 	    }
 
 	    if (defined($vnets)) {
@@ -264,6 +258,17 @@ __PACKAGE__->register_method({
 	    }
 	}
 
+	#always check bridge access
+	my $can_access_vnet = sub {
+	    return 1 if $authuser eq 'root@pam';
+	    return 1 if $rpcenv->check_any($authuser, "/sdn/zones/localnetwork", ['SDN.Audit', 'SDN.Allocate'], 1);
+	    return 1 if $rpcenv->check_sdn_bridge($authuser, "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1);
+	};
+	for my $k (sort keys $ifaces->%*) {
+	    my $type = $ifaces->{$k}->{type};
+	    delete $ifaces->{$k} if ($type eq 'bridge' || $type eq 'OVSBridge') && !$can_access_vnet->($k);
+	}
+
 	return PVE::RESTHandler::hash_to_array($ifaces, 'iface');
    }});
 
-- 
2.30.2

Re: [pve-devel] [PATCH-SERIE pve-access-control/pve-manager/qemu-server] check permissions on local bridge

[Fabian Grünbichler]

On June 5, 2023 1:37 am, Alexandre Derumier wrote:
> add vnet/localbridge permissions management
> 
> Hi,
> as we has discuted some weeks ago,
> this patche serie introduce management of acl for vnets && local bridges
> 
> I have reuse current sdn permissions path, to have common paths
> 
> /sdn/vnets/<zone>/<vnet>
> 
> where the local vmbr are in a virtual "localnetwork" zone
> 
> /sdn/vnets/local/<vnet>
> 
> Vlans permissions  are also handled with
> /sdn/vnets/<zone>/<vnet>/<tag>

these paths don't match the patches ;)

if the paths were like this, then we could go one step further and
admins could set propagate on the zone to hand out access to the full
zone, including all vnets *and* vlan tags, and we could just check the
vnet (or vnet+tag), and the zone would be implicitly checked as well (by
virtue of traversing the ACL path).

we'd need to check for consistency of zone+vnet when checking ACLs
though, which is not required right now.

> 
> if user have permissions on the zone, he have access to all vnets/vlan
> if user have permissions on the vnet/tag, he have access to only the specific vlan.
> if user have permissions on the vnet, he have access to all vlans of the vnet

these last two I'd do differently.

permission on vnet/tag => permission to use that vlan
permission on vnet => permission to use the vnet/bridge (without tag)

if I want to give permission for all tags, I can simply give out the
role on vnet with propagation. since the permissions are only checked
when (re)configuring a guest, it doesn't matter that that check is a bit
expensive/potentially checking a lot of paths..

> 
> I have reworked the sdn zone panel from the tree, to manage permissions
> on displayed vnets.
> 
> some screenshots:
> 
> https://mutulin1.odiso.net/sdnzone-perm.png
> https://mutulin1.odiso.net/localzone-perm.png

I didn't check the GUI patches in detail yet, but IMHO they are also
less important right now (they are only a convenience feature for the
new feature of configuring VLAN access).

we'd like to get the basic patches in place this week if possible, if
that is too soon I can also fold in some of my suggestions as
follow-ups, just tell me what works for you!

> for proxmox7: (for users be able to add permissions before upgrade to pve8)
> pve-access-control: patch1 (to new /vnet/vlan path)
> pve-manager : patch1-2 for the new gui

the access control changes should be enough, it's always possible to set
the ACLs using the regular ACL GUI and/or `pveum`. it might make sense
to have at least the local bridge ACL path (for the zone, or for the
zone and the bridges?) in the regular ACL selectors in 7.x as well, if
we pull in something in pve-manager, than IMHO it should be that, not
the full-flegded new panels.

I do think we need a second pve-access-control patch though (for a new
SDN.Use privilege and corresponding role), that also needs to go into
7.x

> changelog v2:
>  - use /vnets/vlan instead /vnets.vlan
>  - rework the bridge filtering when user have access only to a specific vlan
>  - api2 network: always check bridge access if no filter is defined
> 
> todo:
>  - add permissions on clone/restore ?
> 
> 
> 
> pve-access-control:
> 
> Alexandre Derumier (2):
>   access control: add /sdn/vnets/<vnet>/<vlan> path
>   rpcenvironnment: add check_sdn_bridge
> 
>  src/PVE/AccessControl.pm  |  1 +
>  src/PVE/RPCEnvironment.pm | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> 
> pve-manager:
> 
> Alexandre Derumier (3):
>   add vnet permissions panel
>   add permissions management for "localnetwork" zone
>   api2: network: check permissions for local bridges
> 
>  PVE/API2/Cluster.pm                  |  12 ++
>  PVE/API2/Network.pm                  |  26 ++-
>  www/manager6/Makefile                |   2 +
>  www/manager6/sdn/Browser.js          |  17 +-
>  www/manager6/sdn/VnetACLView.js      | 299 +++++++++++++++++++++++++++
>  www/manager6/sdn/ZoneContentPanel.js |  41 ++++
>  www/manager6/sdn/ZoneContentView.js  |  52 ++++-
>  7 files changed, 420 insertions(+), 29 deletions(-)
>  create mode 100644 www/manager6/sdn/VnetACLView.js
>  create mode 100644 www/manager6/sdn/ZoneContentPanel.js
> 
> qemu-server:
> 
> Alexandre Derumier (1):
>   api2: add check_bridge_access for create/update vm
> 
>  PVE/API2/Qemu.pm | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH-SERIE pve-access-control/pve-manager/qemu-server] check permissions on local bridge

[DERUMIER, Alexandre]

Le lundi 05 juin 2023 à 12:13 +0200, Fabian Grünbichler a écrit :
> On June 5, 2023 1:37 am, Alexandre Derumier wrote:
> > add vnet/localbridge permissions management
> > 
> > Hi,
> > as we has discuted some weeks ago,
> > this patche serie introduce management of acl for vnets && local
> > bridges
> > 
> > I have reuse current sdn permissions path, to have common paths
> > 
> > /sdn/vnets/<zone>/<vnet>
> > 
> > where the local vmbr are in a virtual "localnetwork" zone
> > 
> > /sdn/vnets/local/<vnet>
> > 
> > Vlans permissions  are also handled with
> > /sdn/vnets/<zone>/<vnet>/<tag>
> 
> these paths don't match the patches ;)
> 
> if the paths were like this, then we could go one step further and
> admins could set propagate on the zone to hand out access to the full
> zone, including all vnets *and* vlan tags, and we could just check
> the
> vnet (or vnet+tag), and the zone would be implicitly checked as well
> (by
> virtue of traversing the ACL path).
> 
> we'd need to check for consistency of zone+vnet when checking ACLs
> though, which is not required right now.
oh yes, I think it was my first try. 

currently the vnets id are unique (and possibly (at least in sdn) user
could move the vnet between zones. (not implemented, but technically,
it'll work, and ifreload is able to online replug the vnet with vm
guest running).

I don't think it something that user want to do regulary, so maybe it's
not a problem to use /zone/vnet/tag and It's more secure if users need
to recheck the acl.

 

> 
> > 
> > if user have permissions on the zone, he have access to all
> > vnets/vlan
> > if user have permissions on the vnet/tag, he have access to only
> > the specific vlan.
> > if user have permissions on the vnet, he have access to all vlans
> > of the vnet
> 
> these last two I'd do differently.
> 
> permission on vnet/tag => permission to use that vlan
> permission on vnet => permission to use the vnet/bridge (without tag)
> 

yes, make sense

> if I want to give permission for all tags, I can simply give out the
> role on vnet with propagation. since the permissions are only checked
> when (re)configuring a guest, it doesn't matter that that check is a
> bit
> expensive/potentially checking a lot of paths..
> 
> > 
> > I have reworked the sdn zone panel from the tree, to manage
> > permissions
> > on displayed vnets.
> > 
> > some screenshots:
> > 
> I didn't check the GUI patches in detail yet, but IMHO they are also
> less important right now (they are only a convenience feature for the
> new feature of configuring VLAN access).

Note that the only way currently to create them  for local bridge is in
the datacenter permissions panel. (permissions on sdn zone can already
be done globally).  The gui add support for permissions on both vnet &&
vlan. (and new localnetwork zone)

> 
> we'd like to get the basic patches in place this week if possible, if
> that is too soon I can also fold in some of my suggestions as
> follow-ups, just tell me what works for you!
> 
> > for proxmox7: (for users be able to add permissions before upgrade
> > to pve8)
> > pve-access-control: patch1 (to new /vnet/vlan path)
> > pve-manager : patch1-2 for the new gui
> 
> the access control changes should be enough, it's always possible to
> set
> the ACLs using the regular ACL GUI and/or `pveum`. it might make
> sense
> 
ok, no problem. (Some doc before upgrade should be enough)


> to have at least the local bridge ACL path (for the zone, or for the
> zone and the bridges?) in the regular ACL selectors in 7.x as well,
> if
> we pull in something in pve-manager, than IMHO it should be that, not
> the full-flegded new panels.
I'll look to rewrok the selector, vnets are not yet displayed. (only
sdn zones, and localnetwork zone is also not displayed )

> 
> I do think we need a second pve-access-control patch though (for a
> new
> SDN.Use privilege and corresponding role), that also needs to go into
> 7.x
ok. (I was not sure if Audit was enough, but SDN.Use make sense
indeed).


> 
> > changelog v2:
> >  - use /vnets/vlan instead /vnets.vlan
> >  - rework the bridge filtering when user have access only to a
> > specific vlan
> >  - api2 network: always check bridge access if no filter is defined
> > 
> > todo:
> >  - add permissions on clone/restore ?
> > 
> > 
> > 
> > pve-access-control:
> > 
> > Alexandre Derumier (2):
> >   access control: add /sdn/vnets/<vnet>/<vlan> path
> >   rpcenvironnment: add check_sdn_bridge
> > 
> >  src/PVE/AccessControl.pm  |  1 +
> >  src/PVE/RPCEnvironment.pm | 17 +++++++++++++++++
> >  2 files changed, 18 insertions(+)
> > 
> > 
> > pve-manager:
> > 
> > Alexandre Derumier (3):
> >   add vnet permissions panel
> >   add permissions management for "localnetwork" zone
> >   api2: network: check permissions for local bridges
> > 
> >  PVE/API2/Cluster.pm                  |  12 ++
> >  PVE/API2/Network.pm                  |  26 ++-
> >  www/manager6/Makefile                |   2 +
> >  www/manager6/sdn/Browser.js          |  17 +-
> >  www/manager6/sdn/VnetACLView.js      | 299
> > +++++++++++++++++++++++++++
> >  www/manager6/sdn/ZoneContentPanel.js |  41 ++++
> >  www/manager6/sdn/ZoneContentView.js  |  52 ++++-
> >  7 files changed, 420 insertions(+), 29 deletions(-)
> >  create mode 100644 www/manager6/sdn/VnetACLView.js
> >  create mode 100644 www/manager6/sdn/ZoneContentPanel.js
> > 
> > qemu-server:
> > 
> > Alexandre Derumier (1):
> >   api2: add check_bridge_access for create/update vm
> > 
> >  PVE/API2/Qemu.pm | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> > 
> > 
> > -- 
> > 2.30.2
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://antiphishing.cetsi.fr/proxy/v3?i=d1l4NXNNaWE4SWZqU0dLWcuTfdxEd98NfWIp9dma5kY&r=MXJUa0FrUVJqc1UwYWxNZ-tmXdGQOM0bQR6kYEgvrmGZrgAumkB5XEgd10kSzvIx&f=c2xMdVN4Smh2R2tOZDdIRKCk7WEocHpTPMerT1Q-Aq5qwr8l2xvAWuOGvFsV3frp2oSAgxNUQCpJDHp2iUmTWg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=fjzS
> > 
> > 
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=d1l4NXNNaWE4SWZqU0dLWcuTfdxEd98NfWIp9dma5kY&r=MXJUa0FrUVJqc1UwYWxNZ-tmXdGQOM0bQR6kYEgvrmGZrgAumkB5XEgd10kSzvIx&f=c2xMdVN4Smh2R2tOZDdIRKCk7WEocHpTPMerT1Q-Aq5qwr8l2xvAWuOGvFsV3frp2oSAgxNUQCpJDHp2iUmTWg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=fjzS
> 

Re: [pve-devel] [PATCH-SERIE pve-access-control/pve-manager/qemu-server] check permissions on local bridge

[DERUMIER, Alexandre]

Le mardi 06 juin 2023 à 05:32 +0000, DERUMIER, Alexandre a écrit :
> > to have at least the local bridge ACL path (for the zone, or for
> > the
> > zone and the bridges?) in the regular ACL selectors in 7.x as well,
> > if
> > we pull in something in pve-manager, than IMHO it should be that,
> > not
> > the full-flegded new panels.
> I'll look to rewrok the selector, vnets are not yet displayed. (only
> sdn zones, and localnetwork zone is also not displayed )

Looking at that,
currently the pathselector is filled from cluster ressources.

I really don't known if we want to add all vnets to the cluster
ressources api. (as we don't really want to expose them in the tree
anyway,  like a disk from a datastore is not exposed. User could have a
lot bridges, and the ressource json can be already big)

User could still add manually the bridge in the path if needed.

User could still use the new panel for easy add more granular
permissions on bridge/vlan.

?

Re: [pve-devel] [PATCH-SERIE pve-access-control/pve-manager/qemu-server] check permissions on local bridge

[Fabian Grünbichler]

On June 6, 2023 8:54 am, DERUMIER, Alexandre wrote:
> Le mardi 06 juin 2023 à 05:32 +0000, DERUMIER, Alexandre a écrit :
>> > to have at least the local bridge ACL path (for the zone, or for
>> > the
>> > zone and the bridges?) in the regular ACL selectors in 7.x as well,
>> > if
>> > we pull in something in pve-manager, than IMHO it should be that,
>> > not
>> > the full-flegded new panels.
>> I'll look to rewrok the selector, vnets are not yet displayed. (only
>> sdn zones, and localnetwork zone is also not displayed )
> 
> Looking at that,
> currently the pathselector is filled from cluster ressources.
> 
> I really don't known if we want to add all vnets to the cluster
> ressources api. (as we don't really want to expose them in the tree
> anyway,  like a disk from a datastore is not exposed. User could have a
> lot bridges, and the ressource json can be already big)
> 
> User could still add manually the bridge in the path if needed.
> 
> User could still use the new panel for easy add more granular
> permissions on bridge/vlan.

I think for PVE 7, we mainly want to provide an easy way to stay
compatible to the old behaviour, that would basically mean:

pveum acl modify /sdn/zones/localnetwork --groups XXX --users YYY --tokens ZZZ --roles PVESDNUser

rinse repeat for any SDN zones if you have any.

for the GUI, the least invasive change would be to just add a hardcoded
ACL selector entry for /sdn/zones/localnetwork (the rest can be done
manually via the GUI or pveum).

note that existing guests continue to work fine, it's just
- creating new ones (from scratch, backup or via clone)
- modifying any virtual nics on any guests

that require the permissions.

PVEAdmins and existing PVESDNAdmins would already have the required
permissions anyway (Administrator as well). so this would mostly affect
PVEVMAdmins and custom roles.

Re: [pve-devel] [PATCH-SERIE pve-access-control/pve-manager/qemu-server] check permissions on local bridge

[Fabian Grünbichler]

On June 6, 2023 7:32 am, DERUMIER, Alexandre wrote:
> Le lundi 05 juin 2023 à 12:13 +0200, Fabian Grünbichler a écrit :
>> On June 5, 2023 1:37 am, Alexandre Derumier wrote:
>> > add vnet/localbridge permissions management
>> > 
>> > Hi,
>> > as we has discuted some weeks ago,
>> > this patche serie introduce management of acl for vnets && local
>> > bridges
>> > 
>> > I have reuse current sdn permissions path, to have common paths
>> > 
>> > /sdn/vnets/<zone>/<vnet>
>> > 
>> > where the local vmbr are in a virtual "localnetwork" zone
>> > 
>> > /sdn/vnets/local/<vnet>
>> > 
>> > Vlans permissions  are also handled with
>> > /sdn/vnets/<zone>/<vnet>/<tag>
>> 
>> these paths don't match the patches ;)
>> 
>> if the paths were like this, then we could go one step further and
>> admins could set propagate on the zone to hand out access to the full
>> zone, including all vnets *and* vlan tags, and we could just check
>> the
>> vnet (or vnet+tag), and the zone would be implicitly checked as well
>> (by
>> virtue of traversing the ACL path).
>> 
>> we'd need to check for consistency of zone+vnet when checking ACLs
>> though, which is not required right now.
> oh yes, I think it was my first try. 
> 
> currently the vnets id are unique (and possibly (at least in sdn) user
> could move the vnet between zones. (not implemented, but technically,
> it'll work, and ifreload is able to online replug the vnet with vm
> guest running).
> 
> I don't think it something that user want to do regulary, so maybe it's
> not a problem to use /zone/vnet/tag and It's more secure if users need
> to recheck the acl.

I just wanted to mention it since it caught my eye, treating zones and
vnets as independent also makes sense, it should just be consistent :)


there are pros and cons for both approaches:
- pro for current approach:
-- vnets can be moved/converted between zones, ACLs stay valid
-- no extra checks needed
- pro for zone/vnet/.. approach:
-- propagation from zone to vnet is possible without manually doing it
at the check site
-- binding between zone and vnet is enforced at the ACL level