* [pve-devel] [PATCH pve-manager 0/4] add vnet/localbridge permissions management
@ 2023-05-26 7:27 Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 1/4] add vnet permissions panel Alexandre Derumier
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Alexandre Derumier @ 2023-05-26 7:27 UTC (permalink / raw)
To: pve-devel
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 "local" 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, he have access to all vlans of the vnet
if user have permissions on the vnet.tag, he have access to only the specific vlan.
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
patch1-2: can be applied on proxmox7, so users can already add permissions
before upgrade to proxmox8
patch3-4: add filtering the displayed local bridges (for proxmox8)
Alexandre Derumier (4):
add vnet permissions panel
add permissions management for "local" network zone
api2: network: check permissions for local bridges
api2: network: check vlan permissions for local bridges
PVE/API2/Cluster.pm | 12 ++
PVE/API2/Network.pm | 33 ++-
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, 429 insertions(+), 27 deletions(-)
create mode 100644 www/manager6/sdn/VnetACLView.js
create mode 100644 www/manager6/sdn/ZoneContentPanel.js
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pve-devel] [PATCH pve-manager 1/4] add vnet permissions panel
2023-05-26 7:27 [pve-devel] [PATCH pve-manager 0/4] add vnet/localbridge permissions management Alexandre Derumier
@ 2023-05-26 7:27 ` Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone Alexandre Derumier
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Alexandre Derumier @ 2023-05-26 7:27 UTC (permalink / raw)
To: pve-devel
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..5a0f1762
--- /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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone
2023-05-26 7:27 [pve-devel] [PATCH pve-manager 0/4] add vnet/localbridge permissions management Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 1/4] add vnet permissions panel Alexandre Derumier
@ 2023-05-26 7:27 ` Alexandre Derumier
2023-06-02 11:39 ` Fabian Grünbichler
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 3/4] api2: network: check permissions for local bridges Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan " Alexandre Derumier
3 siblings, 1 reply; 11+ messages in thread
From: Alexandre Derumier @ 2023-05-26 7:27 UTC (permalink / raw)
To: pve-devel
add a default virtual zone called 'local' in the ressource tree,
and handle permissions like a true sdn zone
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..e8f5e06e 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -474,6 +474,18 @@ __PACKAGE__->register_method({
}
}
+ #add default "local" network zone
+ foreach my $node (@$nodelist) {
+ my $local_sdn = {
+ id => "sdn/$node/local",
+ sdn => 'local',
+ 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..1a994fc9 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 === 'local') {
+ 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pve-devel] [PATCH pve-manager 3/4] api2: network: check permissions for local bridges
2023-05-26 7:27 [pve-devel] [PATCH pve-manager 0/4] add vnet/localbridge permissions management Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 1/4] add vnet permissions panel Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone Alexandre Derumier
@ 2023-05-26 7:27 ` Alexandre Derumier
2023-06-02 11:39 ` Fabian Grünbichler
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan " Alexandre Derumier
3 siblings, 1 reply; 11+ messages in thread
From: Alexandre Derumier @ 2023-05-26 7:27 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
PVE/API2/Network.pm | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index b3faba1a..ba3b3e0e 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -240,22 +240,20 @@ __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)
+ #check access for local bridges
+ my $can_access_vnet = sub {
+ return 1 if $authuser eq 'root@pam';
+ return 1 if $rpcenv->check_any($authuser, "/sdn/zones/local", ['SDN.Audit', 'SDN.Allocate'], 1);
+ return 1 if $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));
}
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan permissions for local bridges
2023-05-26 7:27 [pve-devel] [PATCH pve-manager 0/4] add vnet/localbridge permissions management Alexandre Derumier
` (2 preceding siblings ...)
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 3/4] api2: network: check permissions for local bridges Alexandre Derumier
@ 2023-05-26 7:27 ` Alexandre Derumier
2023-06-02 11:39 ` Fabian Grünbichler
3 siblings, 1 reply; 11+ messages in thread
From: Alexandre Derumier @ 2023-05-26 7:27 UTC (permalink / raw)
To: pve-devel
We need to display the bridge is the user have a permission
on any vlan on the bridge.
to avoid to check permissions on 4096 vlans for each bridge
(could be slow with a lot of bridges),
we first list vlans where acls are defined.
(4000 check took 60ms on 10year xeon server, should be enough
for any network where the total number of vlans is limited)
Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
PVE/API2/Network.pm | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index ba3b3e0e..39f17d14 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -240,17 +240,35 @@ __PACKAGE__->register_method({
if (my $tfilter = $param->{type}) {
my $vnets;
+ my $bridges_vlans_acl = {};
#check access for local bridges
my $can_access_vnet = sub {
+ my $bridge = $_[0];
return 1 if $authuser eq 'root@pam';
return 1 if $rpcenv->check_any($authuser, "/sdn/zones/local", ['SDN.Audit', 'SDN.Allocate'], 1);
- return 1 if $rpcenv->check_any($authuser, "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1);
+ return 1 if $rpcenv->check($authuser, "/sdn/vnets/$bridge", ['SDN.Audit'], 1);
+ my $bridge_vlan = $bridges_vlans_acl->{$bridge};
+ for my $tag (sort keys %$bridge_vlan) {
+ return 1 if $rpcenv->check($authuser, "/sdn/vnets/$bridge.$tag", ['SDN.Audit'], 1);
+ }
};
if ($have_sdn && $param->{type} eq 'any_bridge') {
$vnets = PVE::Network::SDN::get_local_vnets(); # returns already access-filtered
}
+ #find all vlans where we have specific acls
+ if ($tfilter =~ /^any(_local)?_bridge$/) {
+ my $cfg = $rpcenv->{user_cfg};
+ my $vnets_acl_root = $cfg->{acl_root}->{children}->{sdn}->{children}->{vnets};
+ PVE::AccessControl::iterate_acl_tree("/", $vnets_acl_root, sub {
+ my ($path, $node) = @_;
+ if ($path =~ /\/(.*)\.(\d+)$/) {
+ $bridges_vlans_acl->{$1}->{$2} = 1;
+ }
+ });
+ }
+
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'));
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone Alexandre Derumier
@ 2023-06-02 11:39 ` Fabian Grünbichler
2023-06-02 12:21 ` DERUMIER, Alexandre
0 siblings, 1 reply; 11+ messages in thread
From: Fabian Grünbichler @ 2023-06-02 11:39 UTC (permalink / raw)
To: Proxmox VE development discussion
On May 26, 2023 9:27 am, Alexandre Derumier wrote:
> add a default virtual zone called 'local' in the ressource tree,
> and handle permissions like a true sdn zone
>
> 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..e8f5e06e 100644
> --- a/PVE/API2/Cluster.pm
> +++ b/PVE/API2/Cluster.pm
> @@ -474,6 +474,18 @@ __PACKAGE__->register_method({
> }
> }
>
> + #add default "local" network zone
> + foreach my $node (@$nodelist) {
> + my $local_sdn = {
> + id => "sdn/$node/local",
> + sdn => 'local',
> + node => $node,
> + type => 'sdn',
> + status => 'ok',
> + };
> + push @$res, $local_sdn;
> + }
> +
should this als obe guarded by the type check like below? is there
anything that ensures that a 'local' zone doesn't already exist as
regular SDN-managed zone?
> 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'));
this line is getting a bit long, maybe it could be reformated or
refactored?
> +
nit: this blank newline is introduced here and removed in the next patch ;)
> 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..1a994fc9 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 === 'local') {
> + 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 mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pve-devel] [PATCH pve-manager 3/4] api2: network: check permissions for local bridges
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 3/4] api2: network: check permissions for local bridges Alexandre Derumier
@ 2023-06-02 11:39 ` Fabian Grünbichler
0 siblings, 0 replies; 11+ messages in thread
From: Fabian Grünbichler @ 2023-06-02 11:39 UTC (permalink / raw)
To: Proxmox VE development discussion
On May 26, 2023 9:27 am, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
> PVE/API2/Network.pm | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> index b3faba1a..ba3b3e0e 100644
> --- a/PVE/API2/Network.pm
> +++ b/PVE/API2/Network.pm
> @@ -240,22 +240,20 @@ __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)
> + #check access for local bridges
> + my $can_access_vnet = sub {
> + return 1 if $authuser eq 'root@pam';
> + return 1 if $rpcenv->check_any($authuser, "/sdn/zones/local", ['SDN.Audit', 'SDN.Allocate'], 1);
> + return 1 if $rpcenv->check_any($authuser, "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1);
here the same question arises - is there anything guarding against name
collisions between SDN vnets and local bridges that pretend to be vnets?
;)
> };
>
> 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));
> }
>
> --
> 2.30.2
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan permissions for local bridges
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan " Alexandre Derumier
@ 2023-06-02 11:39 ` Fabian Grünbichler
2023-06-02 12:32 ` DERUMIER, Alexandre
0 siblings, 1 reply; 11+ messages in thread
From: Fabian Grünbichler @ 2023-06-02 11:39 UTC (permalink / raw)
To: Proxmox VE development discussion
On May 26, 2023 9:27 am, Alexandre Derumier wrote:
> We need to display the bridge is the user have a permission
> on any vlan on the bridge.
>
> to avoid to check permissions on 4096 vlans for each bridge
> (could be slow with a lot of bridges),
> we first list vlans where acls are defined.
>
> (4000 check took 60ms on 10year xeon server, should be enough
> for any network where the total number of vlans is limited)
some sort of spec here for how the ACL looks like would be nice to have
(while it's possible to reverse it from the code, it's always nicer to
have the expection explicit as well).
>
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
> PVE/API2/Network.pm | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> index ba3b3e0e..39f17d14 100644
> --- a/PVE/API2/Network.pm
> +++ b/PVE/API2/Network.pm
> @@ -240,17 +240,35 @@ __PACKAGE__->register_method({
>
> if (my $tfilter = $param->{type}) {
> my $vnets;
> + my $bridges_vlans_acl = {};
> #check access for local bridges
> my $can_access_vnet = sub {
> + my $bridge = $_[0];
> return 1 if $authuser eq 'root@pam';
> return 1 if $rpcenv->check_any($authuser, "/sdn/zones/local", ['SDN.Audit', 'SDN.Allocate'], 1);
> - return 1 if $rpcenv->check_any($authuser, "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1);
> + return 1 if $rpcenv->check($authuser, "/sdn/vnets/$bridge", ['SDN.Audit'], 1);
why does this drop the Allocate? we usually have the "more privileged"
privilege in addition to Audit (if there is one).
> + my $bridge_vlan = $bridges_vlans_acl->{$bridge};
> + for my $tag (sort keys %$bridge_vlan) {
> + return 1 if $rpcenv->check($authuser, "/sdn/vnets/$bridge.$tag", ['SDN.Audit'], 1);
wouldn't $bridge/$tag be more natural? it would allow propagation from a
bridge to all tags using the usual propagate flag as well..
this could also live in pve-access-control as a special helper, then we
wouldn't need to do this dance here (it would be the first
iterate_acl_tree call site outside of pve-access-control!).
something like this in PVE::RPCEnvironment:
sub check_sdn_vlan(.., $bridge, $priv) {
.. iterate over all vlans and check while iterating, returning early for first one with access
}
basically:
my $bridge = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, "/sdn/vnets/$bridge");
if ($bridge) {
for my $vlan (keys $bridge->{children}->%$) {
return 1 if $self->check_any(...);
}
return 1 if # check propagate on bridge itself
}
return 0;
> + }
> };
>
> if ($have_sdn && $param->{type} eq 'any_bridge') {
> $vnets = PVE::Network::SDN::get_local_vnets(); # returns already access-filtered
> }
>
> + #find all vlans where we have specific acls
> + if ($tfilter =~ /^any(_local)?_bridge$/) {
> + my $cfg = $rpcenv->{user_cfg};
> + my $vnets_acl_root = $cfg->{acl_root}->{children}->{sdn}->{children}->{vnets};
> + PVE::AccessControl::iterate_acl_tree("/", $vnets_acl_root, sub {
> + my ($path, $node) = @_;
> + if ($path =~ /\/(.*)\.(\d+)$/) {
> + $bridges_vlans_acl->{$1}->{$2} = 1;
> + }
> + });
> + }
because this iterates over *all* ACLs, only to then return upon the
first match above in $can_access_vnet..
it should also be
iterate_acl_tree("/sdn/vnets", ..) or "/sdn/vnets/$bridge" if vlan tags
live as children of the bridge (path and the node should match!). there
also is "find_acl_tree_node" so you don't need to make assumptions about
how the nodes are laid out.
I do wonder whether something that supports ranges would be more
appropriate rather then encoding this in ACLs directly though.. could
always be added later on though (e.g., named vlan objects that define a
set of vlans).
> +
> 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'));
> --
> 2.30.2
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone
2023-06-02 11:39 ` Fabian Grünbichler
@ 2023-06-02 12:21 ` DERUMIER, Alexandre
2023-06-02 14:18 ` DERUMIER, Alexandre
0 siblings, 1 reply; 11+ messages in thread
From: DERUMIER, Alexandre @ 2023-06-02 12:21 UTC (permalink / raw)
To: pve-devel
Le vendredi 02 juin 2023 à 13:39 +0200, Fabian Grünbichler a écrit :
> On May 26, 2023 9:27 am, Alexandre Derumier wrote:
> > add a default virtual zone called 'local' in the ressource tree,
> > and handle permissions like a true sdn zone
> >
> > 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..e8f5e06e 100644
> > --- a/PVE/API2/Cluster.pm
> > +++ b/PVE/API2/Cluster.pm
> > @@ -474,6 +474,18 @@ __PACKAGE__->register_method({
> > }
> > }
> >
> > + #add default "local" network zone
> > + foreach my $node (@$nodelist) {
> > + my $local_sdn = {
> > + id => "sdn/$node/local",
> > + sdn => 'local',
> > + node => $node,
> > + type => 'sdn',
> > + status => 'ok',
> > + };
> > + push @$res, $local_sdn;
> > + }
> > +
>
> should this als obe guarded by the type check like below? is there
> anything that ensures that a 'local' zone doesn't already exist as
> regular SDN-managed zone?
I was more thinking to forbid "local" name in sdn code in another
patch,
as sdn it's still in beta anyway, user could still rename zone manually
in cfg.
if not, user will not be able to manage local bridges permissions.
>
> > 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'));
>
> this line is getting a bit long, maybe it could be reformated or
> refactored?
yes sure.
>
> > +
>
> nit: this blank newline is introduced here and removed in the next
> patch ;)
>
> > 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..1a994fc9 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 === 'local') {
> > + 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 mailing list
> > pve-devel@lists.proxmox.com
> > https://antiphishing.cetsi.fr/proxy/v3?i=Zk92VEFKaGQ4Ums4cnZEUWMTpfHaXFQGRw1_CnOoOH0&r=bHA1dGV3NWJQVUloaWNFUZPu0fK2BfWNnEHaDLDwG0rtDedpluZBIffSL1M5cj3F&f=SlhDbE9uS2laS2JaZFpNWvmsxai1zlJP9llgnl5HIv-4jAji8Dh2BQawzxID5bzr6Uv-3EQd-eluQbsPfcUOTg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=XRKU
> >
> >
> >
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=Zk92VEFKaGQ4Ums4cnZEUWMTpfHaXFQGRw1_CnOoOH0&r=bHA1dGV3NWJQVUloaWNFUZPu0fK2BfWNnEHaDLDwG0rtDedpluZBIffSL1M5cj3F&f=SlhDbE9uS2laS2JaZFpNWvmsxai1zlJP9llgnl5HIv-4jAji8Dh2BQawzxID5bzr6Uv-3EQd-eluQbsPfcUOTg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=XRKU
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan permissions for local bridges
2023-06-02 11:39 ` Fabian Grünbichler
@ 2023-06-02 12:32 ` DERUMIER, Alexandre
0 siblings, 0 replies; 11+ messages in thread
From: DERUMIER, Alexandre @ 2023-06-02 12:32 UTC (permalink / raw)
To: pve-devel
Le vendredi 02 juin 2023 à 13:39 +0200, Fabian Grünbichler a écrit :
> On May 26, 2023 9:27 am, Alexandre Derumier wrote:
> > We need to display the bridge is the user have a permission
> > on any vlan on the bridge.
> >
> > to avoid to check permissions on 4096 vlans for each bridge
> > (could be slow with a lot of bridges),
> > we first list vlans where acls are defined.
> >
> > (4000 check took 60ms on 10year xeon server, should be enough
> > for any network where the total number of vlans is limited)
>
> some sort of spec here for how the ACL looks like would be nice to
> have
> (while it's possible to reverse it from the code, it's always nicer
> to
> have the expection explicit as well).
>
> >
> > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > ---
> > PVE/API2/Network.pm | 20 +++++++++++++++++++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> > index ba3b3e0e..39f17d14 100644
> > --- a/PVE/API2/Network.pm
> > +++ b/PVE/API2/Network.pm
> > @@ -240,17 +240,35 @@ __PACKAGE__->register_method({
> >
> > if (my $tfilter = $param->{type}) {
> > my $vnets;
> > + my $bridges_vlans_acl = {};
> > #check access for local bridges
> > my $can_access_vnet = sub {
> > + my $bridge = $_[0];
> > return 1 if $authuser eq 'root@pam';
> > return 1 if $rpcenv->check_any($authuser,
> > "/sdn/zones/local", ['SDN.Audit', 'SDN.Allocate'], 1);
> > - return 1 if $rpcenv->check_any($authuser,
> > "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1);
> > + return 1 if $rpcenv->check($authuser,
> > "/sdn/vnets/$bridge", ['SDN.Audit'], 1);
>
> why does this drop the Allocate? we usually have the "more
> privileged"
> privilege in addition to Audit (if there is one).
I think allocate maybe sense on zone. (as what's we allocate are
vnets).
(it should be removed on /sdn/vnets/$_[0]).
But I don't, maybe if user add a simple acl like "/" + propagate with
SDN.allocate only, we want to give it access everywhere ?
>
> > + my $bridge_vlan = $bridges_vlans_acl->{$bridge};
> > + for my $tag (sort keys %$bridge_vlan) {
> > + return 1 if $rpcenv->check($authuser,
> > "/sdn/vnets/$bridge.$tag", ['SDN.Audit'], 1);
>
> wouldn't $bridge/$tag be more natural? it would allow propagation
> from a
> bridge to all tags using the usual propagate flag as well..
>
> this could also live in pve-access-control as a special helper, then
> we
> wouldn't need to do this dance here (it would be the first
> iterate_acl_tree call site outside of pve-access-control!).
>
> something like this in PVE::RPCEnvironment:
>
> sub check_sdn_vlan(.., $bridge, $priv) {
> .. iterate over all vlans and check while iterating, returning
> early for first one with access
> }
>
> basically:
>
> my $bridge = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root},
> "/sdn/vnets/$bridge");
> if ($bridge) {
> for my $vlan (keys $bridge->{children}->%$) {
> return 1 if $self->check_any(...);
> }
> return 1 if # check propagate on bridge itself
> }
> return 0;
>
> > + }
> > };
> >
> > if ($have_sdn && $param->{type} eq 'any_bridge') {
> > $vnets = PVE::Network::SDN::get_local_vnets(); #
> > returns already access-filtered
> > }
> >
> > + #find all vlans where we have specific acls
> > + if ($tfilter =~ /^any(_local)?_bridge$/) {
> > + my $cfg = $rpcenv->{user_cfg};
> > + my $vnets_acl_root = $cfg->{acl_root}->{children}-
> > >{sdn}->{children}->{vnets};
> > + PVE::AccessControl::iterate_acl_tree("/",
> > $vnets_acl_root, sub {
> > + my ($path, $node) = @_;
> > + if ($path =~ /\/(.*)\.(\d+)$/) {
> > + $bridges_vlans_acl->{$1}->{$2} = 1;
> > + }
> > + });
> > + }
>
> because this iterates over *all* ACLs, only to then return upon the
> first match above in $can_access_vnet..
>
> it should also be
>
> iterate_acl_tree("/sdn/vnets", ..) or "/sdn/vnets/$bridge" if vlan
> tags
> live as children of the bridge (path and the node should match!).
> there
> also is "find_acl_tree_node" so you don't need to make assumptions
> about
> how the nodes are laid out.
>
Thanks ! I was really banging my head to implement this properly &&
fast ^_^
> I do wonder whether something that supports ranges would be more
> appropriate rather then encoding this in ACLs directly though..
> could
> always be added later on though (e.g., named vlan objects that define
> a
> set of vlans).
yes, I was thinking the same to avoid too much acl when you need to
access to many vlans.
>
> > +
> > 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'));
> > --
> > 2.30.2
> >
> >
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://antiphishing.cetsi.fr/proxy/v3?i=MlZSTzBhZFZ6Nzl4c3EyN7fbSKDePLMxi5u5_onpAoI&r=cm1qVmRYUWk2WXhYZVFHWA0PXtTaYxz7-FIOTkZBm34_dHdSch-gXn7ST9eGhQLN&f=S1Zkd042VWdrZG5qQUxxWk5ps4t67kNuHsBZzdzhpquLKuXqTZLIq2K1DfKr9N61yBafm7AuAITd6bHtRU4zEQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=F1is
> >
> >
> >
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=MlZSTzBhZFZ6Nzl4c3EyN7fbSKDePLMxi5u5_onpAoI&r=cm1qVmRYUWk2WXhYZVFHWA0PXtTaYxz7-FIOTkZBm34_dHdSch-gXn7ST9eGhQLN&f=S1Zkd042VWdrZG5qQUxxWk5ps4t67kNuHsBZzdzhpquLKuXqTZLIq2K1DfKr9N61yBafm7AuAITd6bHtRU4zEQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=F1is
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone
2023-06-02 12:21 ` DERUMIER, Alexandre
@ 2023-06-02 14:18 ` DERUMIER, Alexandre
0 siblings, 0 replies; 11+ messages in thread
From: DERUMIER, Alexandre @ 2023-06-02 14:18 UTC (permalink / raw)
To: pve-devel
Le vendredi 02 juin 2023 à 12:21 +0000, DERUMIER, Alexandre a écrit :
> > should this als obe guarded by the type check like below? is there
> > anything that ensures that a 'local' zone doesn't already exist as
> > regular SDN-managed zone?
>
> I was more thinking to forbid "local" name in sdn code in another
> patch,
> as sdn it's still in beta anyway, user could still rename zone
> manually
> in cfg.
>
> if not, user will not be able to manage local bridges permissions.
Thinking about this, the sdn zone names are limited to 8 characters,
so I could simply use "localnetwork" or "localbridges" as name
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-06-02 14:19 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 7:27 [pve-devel] [PATCH pve-manager 0/4] add vnet/localbridge permissions management Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 1/4] add vnet permissions panel Alexandre Derumier
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 2/4] add permissions management for "local" network zone Alexandre Derumier
2023-06-02 11:39 ` Fabian Grünbichler
2023-06-02 12:21 ` DERUMIER, Alexandre
2023-06-02 14:18 ` DERUMIER, Alexandre
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 3/4] api2: network: check permissions for local bridges Alexandre Derumier
2023-06-02 11:39 ` Fabian Grünbichler
2023-05-26 7:27 ` [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan " Alexandre Derumier
2023-06-02 11:39 ` Fabian Grünbichler
2023-06-02 12:32 ` DERUMIER, Alexandre
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox