Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
On 4/2/24 22:47, Laurent GUERBY wrote:
> REJECT is a L3 IP feature, to implement it properly in all cases your
> firewall rule needs to know both about IP adresses involved (and the
> corresponding MAC too in the ethernet case).
Yes indeed, although we have L3 and L4 information available in the
bridge table, otherwise REJECT would also be impossible in the
prerouting / input hooks. REJECTing in the bridge table simply sends a
packet with source and destination IP of the initial packet flipped [1].
Nevertheless, we cannot tell whether the destination IP address in the
packet is actually the IP address of the guest (in the case of VMs) -
even if the MAC address is from the guest. So we might need to combine
this with the ipfilter ipsets if we want to make sure to only send
REJECTs with IP addresses that are actually configured in the guest.
> I don't currently use the proxmox VE firewalling capabilities (I was
> waiting for nftables to look at it :) but may be a compromise would be
> to warn during the transition from iptables to nftables (or from
> version N to N+1) that if a REJECT rule is found without explicit IP
> and MAC that it will just be transformed to DROP, and if the user wants
> a REJECT the user needs to add explicit IP and MAC pairs.
See my point above regarding ipfilters.
> Then the Promox VE firewalling can be done in "ip" tables which know
> how to match ether MAC ("type ipv4_addr . ether_addr" to match both IP
> and MAC at the same time) and no firewall bridge needed.
Host firewalling is already done in the inet tables (which is ip + ip6),
but for guest firewalling we will have to use the bridge table one way
or another, since it is the only viable hook for handling the guest
traffic (see [2]), which usually runs via bridges.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/ipv4/netfilter/nf_reject_ipv4.c?h=v6.8.2#n168
[2] https://wiki.nftables.org/wiki-nftables/index.php/Netfilter_hooks
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
On 4/3/24 07:37, DERUMIER, Alexandre via pve-devel wrote: > I'll really take time to test it (I was super busy theses last month > with a datacenter migration), as I wait for nftables since a while. > > Can't help too much with rust, but I really appriciate it, as I had > some servers with a lot of vms && rules, take more than 10s to generate > the rules with current perl implementation). Thanks! I'd be really interested in how this performs with a large ruleset since I haven't really tried with an excessively large ruleset so far. I have only done very basic checks of the performance, but it looked quite promising. 50% of the time is actually spent in libnftables, so I'd be interested to see how this changes with large rulesets. There is also still some room for performance improvements, since performance wasn't my main concern so far. For instance I am currently reading the guest configuration files 1:1 via the filesystem, but I wanted to implement getting the configuration via pmxcfs where one call would then suffice to retrieve the network configuration of all guests. If you have a large configuration I could use for testing, that you'd be willing to share, then I could run tests myself. Otherwise I will probably use a script to generate a huge config myself at some point. > I really would like to not have fwbr bridge anymore, because I have > seen a big performance bug with them: I agree 100%, getting rid of those would eliminate several bugs and issues. > I'll try your code, see the generated rules, and try to see if I can > get reject working. Thanks! Maybe you can come up with something. Otherwise we might have to implement a configuration option that switches between firewall bridge on/off and people have to make choices about the tradeoffs themselves. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox] notify: fix #5274: also set 'X-Gotify-Key' header for authentication
Versions of Gotify < 2.2.0 only supported the 'X-Gotify-Key' header
for passing the API token. This comment sets this header in addition
to the regular 'Authorization' header in order to be compatible with
older Gotify servers.
Signed-off-by: Lukas Wagner
---
proxmox-notify/src/endpoints/gotify.rs | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/proxmox-notify/src/endpoints/gotify.rs
b/proxmox-notify/src/endpoints/gotify.rs
index 90ae959..20c83bf 100644
--- a/proxmox-notify/src/endpoints/gotify.rs
+++ b/proxmox-notify/src/endpoints/gotify.rs
@@ -124,10 +124,13 @@ impl Endpoint for GotifyEndpoint {
let body = serde_json::to_vec(&body)
.map_err(|err| Error::NotifyFailed(self.name().to_string(),
err.into()))?;
-let extra_headers = HashMap::from([(
-"Authorization".into(),
-format!("Bearer {}", self.private_config.token),
-)]);
+let extra_headers = HashMap::from([
+(
+"Authorization".into(),
+format!("Bearer {}", self.private_config.token),
+),
+("X-Gotify-Key".into(), self.private_config.token.clone()),
+]);
let proxy_config = context()
.http_proxy_config()
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
On 4/3/24 08:05, DERUMIER, Alexandre via pve-devel wrote: > Personnaly, I'm not sure than using reject / tcp-reset in a bridged is > a good idea. (Even if personally I'm using it production, I don't have > problem to switch to DROP, if I can avoid other problems) Yes, I tend to agree. But there certainly will be users who want to use REJECT for guest firewalls and it certainly makes sense to support that - especially since we have supported it since the beginning. It's hard to take away features. I also feel like this is something a firewall should support one way or another and if it's not there we are missing a really basic feature. > Maybe it is time to disable dynamic mac-learning by default ? > The code is already here and works fine. > > AFAIK, other hypervisor like vmware disable port flooding by default > with static mac registration too. Might be a good idea, although it still wouldn't solve the problem - sadly (since we're still not allowed to do REJECT then). ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v3 00/30] add automated/unattended installation
On Tue, Apr 02, 2024 at 04:55:11PM +0200, Aaron Lauterer wrote: [..] > > > > - While trying out different configurations, I wondered if for the > >network something like this would be better for static IPs: > > > > [network.manual] > > cidr = ".." > > dns = ".." > > [..] > > > >.. keeping the `network.use_dhcp` option as before. Would simplify > >some checks now and provide good future-proofing for any new options > >that might get added. > > > >Thereby basically modelling > >`proxmox_auto_installer::answer::NetworkSettings` enum nearly 1:1 to > >the TOML config. > > okay, so that in the DHPC case, it could be > [network] > use_dhcp = true > > and in the manual case, either > [network] > manual.cidr = "…" > manual.dns = "…" > > and so forth, or, to keep it simpler, like your example with > [network.manual] defining the overall manual key. Yeah, exactly. > > This will make it slightly more elaborate to document, as we need to dig > deeper into how TOML works and that there are multiple ways to define the > same hierarchy. But it could be worth it to keep the definition cleaner. > > Some more feedback in that regard might be useful, especially since changing > the format later on will be, as you described it, a PITA :) Feel free though to not block this series on further feedback for this! :^) Just came to mind while pondering over this and trying different settings - but doesn't change anything wrt. functionality really. IMO we can change/break the answer file format at least with a new major release later on, so it's not completely set in stone after all. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v3 00/30] add automated/unattended installation
On 2024-04-03 10:19, Christoph Heiss wrote: On Tue, Apr 02, 2024 at 04:55:11PM +0200, Aaron Lauterer wrote: [..] - While trying out different configurations, I wondered if for the network something like this would be better for static IPs: [network.manual] cidr = ".." dns = ".." [..] .. keeping the `network.use_dhcp` option as before. Would simplify some checks now and provide good future-proofing for any new options that might get added. Thereby basically modelling `proxmox_auto_installer::answer::NetworkSettings` enum nearly 1:1 to the TOML config. okay, so that in the DHPC case, it could be [network] use_dhcp = true and in the manual case, either [network] manual.cidr = "…" manual.dns = "…" and so forth, or, to keep it simpler, like your example with [network.manual] defining the overall manual key. Yeah, exactly. This will make it slightly more elaborate to document, as we need to dig deeper into how TOML works and that there are multiple ways to define the same hierarchy. But it could be worth it to keep the definition cleaner. Some more feedback in that regard might be useful, especially since changing the format later on will be, as you described it, a PITA :) Feel free though to not block this series on further feedback for this! :^) Just came to mind while pondering over this and trying different settings - but doesn't change anything wrt. functionality really. IMO we can change/break the answer file format at least with a new major release later on, so it's not completely set in stone after all. Thinking about it a bit more, I would let it be as it is. The current format is nicer for the actual users, and I did implement it the current way with that in mind. If we realize that it is problematic for some reason, we can change it in a future (major) release. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] vSwitch gui
Hi! Please use our bugtracker for feature requests and bug reports [1]. [1] https://bugzilla.proxmox.com ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 1/3] ui: pool members: avoid setting request parameter for all edit windows
Currently, after adding a storage to a pool, opening any edit window
will send a GET request with a superfluous `poolid` parameter and
cause a parameter verification error in the GUI. This breaks all edit
windows of the current session. A workaround is to reload the current
browser session.
This happens because the `PVE.pool.AddStorage` component inadvertently
adds `poolid` to an `extraRequestParams` object that is shared by all
instances of `Proxmox.window.Edit`, affecting all edit windows in the
current session. Fix this by instead creating a new object that is
local to the component.
Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
Signed-off-by: Friedrich Weber
---
Notes:
changes since v1:
- remove unnecessary quotes (thx Stefan)
www/manager6/grid/PoolMembers.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 75f20cab..af6af1bd 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -123,7 +123,7 @@ Ext.define('PVE.pool.AddStorage', {
me.isAdd = true;
me.url = "/pools/";
me.method = 'PUT';
- me.extraRequestParams.poolid = me.pool;
+ me.extraRequestParams = { poolid: me.pool };
Ext.apply(me, {
subject: gettext('Storage'),
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager/widget-toolkit 0/3] ui: avoid UI bugs due to shared extra request params
Currently, `Proxmox.window.Edit` initializes `extraRequestParams` to an object that, if not overwritten, is shared between all instances of subclasses. This bears the danger of modifying the shared object in a subclass instead of overwriting it, which affects all edit windows of the current session and can cause hard-to-catch UI bugs [1] - Patch 1/3 fixes such an UI bug. - Patch 2/3 (optional) fixes other occurrences of the pattern from 1/3, which are not buggy at the moment, but may become in the future. - Patch 3/3 (optional) changes `Proxmox.window.Edit` to make this class of bugs less likely in the future. Changes from v1: - Patch 1/3: avoid unnecessary quotes - Patch 2/3 + 3/3 are new [1] https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html manager: Friedrich Weber (2): ui: pool members: avoid setting request parameter for all edit windows ui: pool members: avoid sharing object for extra request parameters www/manager6/grid/PoolMembers.js | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) widget-toolkit: Friedrich Weber (1): window: edit: avoid shared object for extra request params src/window/Edit.js | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) Summary over all repositories: 2 files changed, 10 insertions(+), 0 deletions(-) -- Generated by git-murpp 0.5.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH widget-toolkit 3/3] window: edit: avoid shared object for extra request params
Currently, `Proxmox.window.Edit` initializes `extraRequestParams` to
an object that, if not overwritten, is shared between all instances of
subclasses. This bears the danger of modifying the shared object in a
subclass instead of overwriting it, which affects all edit windows of
the current session and can cause hard-to-catch UI bugs [1].
To avoid such bugs in the future, initialize `extraRequestParams` to
`undefined` instead, which forces subclasses to initialize their own
objects.
Note that bugs of the same kind can still happen if a subclass
initializes `extraRequestParams` to an empty shared object and
inadvertently modifies it, but at least they will be limited to that
particular subclass.
[1] https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html
Signed-off-by: Friedrich Weber
---
Notes:
With patch 2/3 applied, I think all occurrences of
`extraRequestParams` in PVE/PBS create their own object (PMG does not
seem to use `extraRequestParams`), so this patch should not break
anything, and if it does, it should be quite noticeable.
new in v2
src/window/Edit.js | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/window/Edit.js b/src/window/Edit.js
index d4a2b551..27cd8d01 100644
--- a/src/window/Edit.js
+++ b/src/window/Edit.js
@@ -9,7 +9,7 @@ Ext.define('Proxmox.window.Edit', {
// to submit extra params on load and submit, useful, e.g., if not all ID
// parameters are included in the URL
-extraRequestParams: {},
+extraRequestParams: undefined,
resizable: false,
@@ -80,7 +80,9 @@ Ext.define('Proxmox.window.Edit', {
let me = this;
let values = {};
- Ext.apply(values, me.extraRequestParams);
+ if (me.extraRequestParams) {
+ Ext.apply(values, me.extraRequestParams);
+ }
let form = me.formPanel.getForm();
@@ -209,7 +211,7 @@ Ext.define('Proxmox.window.Edit', {
waitMsgTarget: me,
}, options);
- if (Object.keys(me.extraRequestParams).length > 0) {
+ if (me.extraRequestParams && Object.keys(me.extraRequestParams).length
> 0) {
let params = newopts.params || {};
Ext.applyIf(params, me.extraRequestParams);
newopts.params = params;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 2/3] ui: pool members: avoid sharing object for extra request parameters
Currently, all instances of `PVE.pool.AddVM` in a session share the
same `extraRequestParams` object. Right now, this does not cause any
problems because only one window can be active at a time, and all
relevant keys are always overwritten.
Still, in order to avoid hard-to-catch bugs due to the shared object
in the future, create a new `extraRequestParams` object for each
instance of `PVE.pool.AddVM`.
Signed-off-by: Friedrich Weber
---
Notes:
new in v2
www/manager6/grid/PoolMembers.js | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index af6af1bd..4ffcde7f 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -6,10 +6,6 @@ Ext.define('PVE.pool.AddVM', {
isAdd: true,
isCreate: true,
-extraRequestParams: {
- 'allow-move': 1,
-},
-
initComponent: function() {
var me = this;
@@ -19,7 +15,10 @@ Ext.define('PVE.pool.AddVM', {
me.url = '/pools/';
me.method = 'PUT';
- me.extraRequestParams.poolid = me.pool;
+ me.extraRequestParams = {
+ 'allow-move': 1,
+ poolid: me.pool,
+ };
var vmsField = Ext.create('Ext.form.field.Text', {
name: 'vms',
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-manager] ui: node: extend option editor for wake on lan
Commit 3f83a0332ef5850c7b2324ca5958fa9b4b4dd61c switched the nodes
`wakeonlan` configuration parameter to be a property string and the
subsequent patches added bind-interface and broadcast-address as
additional optional parameters.
Make this editable in the node options, by adding a dedicated editor
component.
The editor is used for the wake on lan record of the grid only, by
adding the `handle_editor` function, to calls the required editor
based on the selected record of the grid.
Signed-off-by: Christian Ebner
---
www/manager6/node/NodeOptionsView.js | 87 +++-
1 file changed, 74 insertions(+), 13 deletions(-)
diff --git a/www/manager6/node/NodeOptionsView.js
b/www/manager6/node/NodeOptionsView.js
index 661c0e90..6a9e3826 100644
--- a/www/manager6/node/NodeOptionsView.js
+++ b/www/manager6/node/NodeOptionsView.js
@@ -1,3 +1,55 @@
+Ext.define('PVE.node.WakeOnLanEdit', {
+extend: 'Proxmox.window.Edit',
+xtype: 'pveNodeWakeOnLanEdit',
+
+subject: gettext('Wake on LAN settings'),
+width: 350,
+
+items: [
+ {
+ xtype: 'textfield',
+ name: 'mac',
+ fieldLabel: gettext('MAC address'),
+ labelWidth: 130,
+ vtype: 'MacAddress',
+ },
+ {
+ xtype: 'textfield',
+ name: 'bind-interface',
+ fieldLabel: gettext('Bind interface'),
+ labelWidth: 130,
+ },
+ {
+ xtype: 'textfield',
+ name: 'broadcast-address',
+ fieldLabel: gettext('Broadcast address'),
+ labelWidth: 130,
+ vtype: 'IPAddress',
+ },
+],
+
+getValues: function() {
+ let me = this;
+ let wolConfig = {
+ mac: me.down('textfield[name=mac]').getValue(),
+ "bind-interface":
me.down('textfield[name=bind-interface]').getValue(),
+ "broadcast-address":
me.down('textfield[name=broadcast-address]').getValue(),
+ };
+ let wolString = PVE.Parser.printPropertyString(wolConfig, 'mac');
+ return { wakeonlan: wolString };
+},
+
+setValues: function(values) {
+ let me = this;
+ if (values.wakeonlan) {
+ let wolConfig = PVE.Parser.parsePropertyString(values.wakeonlan,
'mac');
+ Ext.Object.each(wolConfig, function(name, value) {
+ me.down('textfield[name=' + name + ']').setValue(value);
+ });
+ }
+},
+});
+
Ext.define('Proxmox.node.NodeOptionsView', {
extend: 'Proxmox.grid.ObjectGrid',
alias: ['widget.proxmoxNodeOptionsView'],
@@ -15,8 +67,26 @@ Ext.define('Proxmox.node.NodeOptionsView', {
return {};
},
+handle_editor: function() {
+ let me = this;
+ var selection_model = me.getSelectionModel();
+ let record = selection_model.getSelection()[0];
+ if (record.id === 'wakeonlan') {
+ let baseUrl = `/nodes/${me.nodename}/config`;
+ Ext.create('PVE.node.WakeOnLanEdit', {
+ url: `/api2/extjs/${baseUrl}`,
+ autoLoad: true,
+ listeners: {
+ destroy: function() { me.reload(); },
+ },
+ }).show();
+ } else {
+ me.run_editor();
+ }
+},
+
listeners: {
- itemdblclick: function() { this.run_editor(); },
+ itemdblclick: function() { this.handle_editor(); },
activate: function() { this.rstore.startUpdate(); },
destroy: function() { this.rstore.stopUpdate(); },
deactivate: function() { this.rstore.stopUpdate(); },
@@ -27,7 +97,7 @@ Ext.define('Proxmox.node.NodeOptionsView', {
text: gettext('Edit'),
xtype: 'proxmoxButton',
disabled: true,
- handler: btn => btn.up('grid').run_editor(),
+ handler: btn => btn.up('grid').handle_editor(),
},
],
@@ -52,17 +122,8 @@ Ext.define('Proxmox.node.NodeOptionsView', {
{
xtype: 'text',
name: 'wakeonlan',
- text: gettext('MAC address for Wake on LAN'),
- vtype: 'MacAddress',
- labelWidth: 150,
- deleteEmpty: true,
- renderer: function(value) {
- if (value === undefined) {
- return Proxmox.Utils.NoneText;
- }
-
- return value;
- },
+ text: gettext('Wake on LAN settings'),
+ defaultValue: Proxmox.Utils.noneText,
},
],
});
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer] html: pbs: fix missing in template after feature list
This adds an empty line between the feature list and the "more information" paragraph, which looks a lot better. The exact same is already present in the HTML template for both other products, probably a simple oversight. Signed-off-by: Christoph Heiss --- html/pbs/extract1-license.htm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/html/pbs/extract1-license.htm b/html/pbs/extract1-license.htm index 9ceae4f..cbaf9de 100644 --- a/html/pbs/extract1-license.htm +++ b/html/pbs/extract1-license.htm @@ -21,7 +21,7 @@ - Deduplication - Incremental backups - Remote sync -- Easy management +- Easy management For more information, visit www.proxmox.com or the Proxmox Backup Server project page. -- 2.43.1 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 02/37] config: firewall: add types for ip addresses
On Tue Apr 2, 2024 at 7:15 PM CEST, Stefan Hanreich wrote:
> Includes types for all kinds of IP values that can occur in the
> firewall config. Additionally, FromStr implementations are available
> for parsing from the config files.
>
> Co-authored-by: Wolfgang Bumiller
> Signed-off-by: Stefan Hanreich
> ---
> proxmox-ve-config/src/firewall/mod.rs | 1 +
> .../src/firewall/types/address.rs | 624 ++
> proxmox-ve-config/src/firewall/types/mod.rs | 3 +
> proxmox-ve-config/src/lib.rs | 1 +
> 4 files changed, 629 insertions(+)
> create mode 100644 proxmox-ve-config/src/firewall/mod.rs
> create mode 100644 proxmox-ve-config/src/firewall/types/address.rs
> create mode 100644 proxmox-ve-config/src/firewall/types/mod.rs
>
> diff --git a/proxmox-ve-config/src/firewall/mod.rs
> b/proxmox-ve-config/src/firewall/mod.rs
> new file mode 100644
> index 000..cd40856
> --- /dev/null
> +++ b/proxmox-ve-config/src/firewall/mod.rs
> @@ -0,0 +1 @@
> +pub mod types;
> diff --git a/proxmox-ve-config/src/firewall/types/address.rs
> b/proxmox-ve-config/src/firewall/types/address.rs
> new file mode 100644
> index 000..ce2f1cd
> --- /dev/null
> +++ b/proxmox-ve-config/src/firewall/types/address.rs
> @@ -0,0 +1,624 @@
> +use std::fmt;
> +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
> +use std::ops::Deref;
> +
> +use anyhow::{bail, format_err, Error};
> +use serde_with::DeserializeFromStr;
> +
> +#[derive(Clone, Copy, Debug, Eq, PartialEq)]
> +pub enum Family {
> +V4,
> +V6,
> +}
> +
> +impl fmt::Display for Family {
> +fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +match self {
> +Family::V4 => f.write_str("Ipv4"),
> +Family::V6 => f.write_str("Ipv6"),
> +}
> +}
> +}
> +
> +#[derive(Clone, Debug)]
> +#[cfg_attr(test, derive(Eq, PartialEq))]
> +pub enum Cidr {
> +Ipv4(Ipv4Cidr),
> +Ipv6(Ipv6Cidr),
> +}
> +
> +impl Cidr {
> +pub fn new_v4(addr: impl Into, mask: u8) -> Result Error> {
> +Ok(Cidr::Ipv4(Ipv4Cidr::new(addr, mask)?))
> +}
> +
> +pub fn new_v6(addr: impl Into, mask: u8) -> Result Error> {
> +Ok(Cidr::Ipv6(Ipv6Cidr::new(addr, mask)?))
> +}
> +
> +pub const fn family(&self) -> Family {
> +match self {
> +Cidr::Ipv4(_) => Family::V4,
> +Cidr::Ipv6(_) => Family::V6,
> +}
> +}
> +
> +pub fn is_ipv4(&self) -> bool {
> +matches!(self, Cidr::Ipv4(_))
> +}
> +
> +pub fn is_ipv6(&self) -> bool {
> +matches!(self, Cidr::Ipv6(_))
> +}
> +}
> +
> +impl fmt::Display for Cidr {
> +fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +match self {
> +Self::Ipv4(ip) => f.write_str(ip.to_string().as_str()),
> +Self::Ipv6(ip) => f.write_str(ip.to_string().as_str()),
> +}
> +}
> +}
> +
> +impl std::str::FromStr for Cidr {
> +type Err = Error;
> +
> +fn from_str(s: &str) -> Result {
> +if let Ok(ip) = s.parse::() {
> +return Ok(Cidr::Ipv4(ip));
> +}
> +
> +if let Ok(ip) = s.parse::() {
> +return Ok(Cidr::Ipv6(ip));
> +}
> +
> +bail!("invalid ip address or CIDR: {s:?}");
> +}
> +}
> +
> +impl From for Cidr {
> +fn from(cidr: Ipv4Cidr) -> Self {
> +Cidr::Ipv4(cidr)
> +}
> +}
> +
> +impl From for Cidr {
> +fn from(cidr: Ipv6Cidr) -> Self {
> +Cidr::Ipv6(cidr)
> +}
> +}
> +
> +const IPV4_LENGTH: u8 = 32;
> +
> +#[derive(Clone, Debug)]
> +#[cfg_attr(test, derive(Eq, PartialEq))]
> +pub struct Ipv4Cidr {
> +addr: Ipv4Addr,
> +mask: u8,
> +}
> +
> +impl Ipv4Cidr {
> +pub fn new(addr: impl Into, mask: u8) -> Result {
> +if mask > 32 {
> +bail!("mask out of range for ipv4 cidr ({mask})");
> +}
> +
> +Ok(Self {
> +addr: addr.into(),
> +mask,
> +})
> +}
> +
> +pub fn contains_address(&self, other: &Ipv4Addr) -> bool {
> +let bits = u32::from_be_bytes(self.addr.octets());
> +let other_bits = u32::from_be_bytes(other.octets());
> +
> +let shift_amount: u32 = IPV4_LENGTH.saturating_sub(self.mask).into();
> +
> +bits.checked_shr(shift_amount).unwrap_or(0)
> +== other_bits.checked_shr(shift_amount).unwrap_or(0)
> +}
> +
> +pub fn address(&self) -> &Ipv4Addr {
> +&self.addr
> +}
> +
> +pub fn mask(&self) -> u8 {
> +self.mask
> +}
> +}
> +
> +impl> From for Ipv4Cidr {
> +fn from(value: T) -> Self {
> +Self {
> +addr: value.into(),
> +mask: 32,
> +}
> +}
> +}
> +
> +impl std::str::FromStr for Ipv4Cidr {
> +type Err = Error;
> +
> +fn from_str(s: &str) -> Result {
> +Ok(match s.find('/') {
> +None => Self {
> +addr: s.parse()?,
> +mask: 32,
> +},
> +Some
Re: [pve-devel] [PATCH proxmox-firewall 11/37] config: firewall: add generic parser for firewall configs
On Tue Apr 2, 2024 at 7:16 PM CEST, Stefan Hanreich wrote:
> Since the basic format of cluster, host and guest firewall
> configurations is the same, we create a generic parser that can handle
> the common config format. The main difference is in the available
> options, which can be passed via a generic parameter.
>
> Co-authored-by: Wolfgang Bumiller
> Signed-off-by: Stefan Hanreich
> ---
> proxmox-ve-config/src/firewall/common.rs | 182 +
> proxmox-ve-config/src/firewall/mod.rs| 1 +
> proxmox-ve-config/src/firewall/parse.rs | 200 +++
> 3 files changed, 383 insertions(+)
> create mode 100644 proxmox-ve-config/src/firewall/common.rs
>
> diff --git a/proxmox-ve-config/src/firewall/common.rs
> b/proxmox-ve-config/src/firewall/common.rs
> new file mode 100644
> index 000..887339b
> --- /dev/null
> +++ b/proxmox-ve-config/src/firewall/common.rs
> @@ -0,0 +1,182 @@
> +use std::collections::HashMap;
> +use std::io;
> +
> +use anyhow::{bail, format_err, Error};
> +use serde::de::IntoDeserializer;
> +
> +use crate::firewall::parse::{parse_named_section_tail, split_key_value,
> SomeString};
> +use crate::firewall::types::ipset::{IpsetName, IpsetScope};
> +use crate::firewall::types::{Alias, Group, Ipset, Rule};
> +
> +#[derive(Debug, Default)]
> +pub struct Config
> +where
> +O: Default + std::fmt::Debug + serde::de::DeserializeOwned,
> +{
> +pub(crate) options: O,
> +pub(crate) rules: Vec,
> +pub(crate) aliases: HashMap,
> +pub(crate) ipsets: HashMap,
> +pub(crate) groups: HashMap,
> +}
> +
> +enum Sec {
> +None,
> +Options,
> +Aliases,
> +Rules,
> +Ipset(String, Ipset),
> +Group(String, Group),
> +}
> +
> +#[derive(Default)]
> +pub struct ParserConfig {
> +/// Network interfaces must be of the form `netX`.
> +pub guest_iface_names: bool,
> +pub ipset_scope: Option,
> +}
> +
> +impl Config
> +where
> +O: Default + std::fmt::Debug + serde::de::DeserializeOwned,
> +{
> +pub fn new() -> Self {
> +Self::default()
> +}
> +
> +pub fn parse(input: R, parser_cfg: &ParserConfig) ->
> Result {
> +let mut section = Sec::None;
> +
> +let mut this = Self::new();
> +let mut options = HashMap::new();
> +
> +for line in input.lines() {
> +let line = line?;
> +let line = line.trim();
> +
> +if line.is_empty() || line.starts_with('#') {
> +continue;
> +}
> +
> +if line.eq_ignore_ascii_case("[OPTIONS]") {
> +this.set_section(&mut section, Sec::Options)?;
> +} else if line.eq_ignore_ascii_case("[ALIASES]") {
> +this.set_section(&mut section, Sec::Aliases)?;
> +} else if line.eq_ignore_ascii_case("[RULES]") {
> +this.set_section(&mut section, Sec::Rules)?;
> +} else if let Some(line) = line.strip_prefix("[IPSET") {
> +let (name, comment) = parse_named_section_tail("ipset",
> line)?;
> +
> +let scope = parser_cfg.ipset_scope.ok_or_else(|| {
> +format_err!("IPSET in config, but no scope set in parser
> config")
> +})?;
> +
> +let ipset_name = IpsetName::new(scope, name.to_string());
> +let mut ipset = Ipset::new(ipset_name);
> +ipset.comment = comment.map(str::to_owned);
> +
> +this.set_section(&mut section, Sec::Ipset(name.to_string(),
> ipset))?;
> +} else if let Some(line) = line.strip_prefix("[group") {
> +let (name, comment) = parse_named_section_tail("group",
> line)?;
> +let mut group = Group::new();
> +
> +group.set_comment(comment.map(str::to_owned));
> +
> +this.set_section(&mut section, Sec::Group(name.to_owned(),
> group))?;
> +} else if line.starts_with('[') {
> +bail!("invalid section {line:?}");
> +} else {
> +match &mut section {
> +Sec::None => bail!("config line with no section:
> {line:?}"),
> +Sec::Options => Self::parse_option(line, &mut options)?,
> +Sec::Aliases => this.parse_alias(line)?,
> +Sec::Rules => this.parse_rule(line, parser_cfg)?,
> +Sec::Ipset(_name, ipset) => ipset.parse_entry(line)?,
> +Sec::Group(_name, group) => group.parse_entry(line)?,
> +}
> +}
> +}
> +this.set_section(&mut section, Sec::None)?;
> +
> +this.options = O::deserialize(IntoDeserializer::<
> +'_,
> +crate::firewall::parse::SerdeStringError,
> +>::into_deserializer(options))?;
> +
> +Ok(this)
> +}
> +
> +fn parse_option(line: &str, options: &mut HashMap)
> -> Result<(), Error> {
> +let (key, value) = split_key_valu
Re: [pve-devel] [PATCH proxmox-firewall 06/37] config: host: add helpers for host network configuration
On Tue Apr 2, 2024 at 7:15 PM CEST, Stefan Hanreich wrote:
> Currently the helpers for obtaining the host network configuration
> panic on error, which could be avoided by the use of
> OnceLock::get_or_init, but this method is currently only available in
> nightly versions.
>
> Generally, if there is a problem with obtaining a hostname for the
> current node then something else is probably already quite broken, so
> I would deem it acceptable for now, same goes for obtaining the
> current network configuration.
>
> Co-authored-by: Wolfgang Bumiller
> Signed-off-by: Stefan Hanreich
> ---
> proxmox-ve-config/Cargo.toml| 1 +
> proxmox-ve-config/src/host/mod.rs | 1 +
> proxmox-ve-config/src/host/utils.rs | 97 +
> proxmox-ve-config/src/lib.rs| 1 +
> 4 files changed, 100 insertions(+)
> create mode 100644 proxmox-ve-config/src/host/mod.rs
> create mode 100644 proxmox-ve-config/src/host/utils.rs
>
> diff --git a/proxmox-ve-config/Cargo.toml b/proxmox-ve-config/Cargo.toml
> index 7bb391e..480eb58 100644
> --- a/proxmox-ve-config/Cargo.toml
> +++ b/proxmox-ve-config/Cargo.toml
> @@ -13,6 +13,7 @@ license = "AGPL-3"
> [dependencies]
> log = "0.4"
> anyhow = "1"
> +nix = "0.26"
>
> serde = { version = "1", features = [ "derive" ] }
> serde_json = "1"
> diff --git a/proxmox-ve-config/src/host/mod.rs
> b/proxmox-ve-config/src/host/mod.rs
> new file mode 100644
> index 000..b5614dd
> --- /dev/null
> +++ b/proxmox-ve-config/src/host/mod.rs
> @@ -0,0 +1 @@
> +pub mod utils;
> diff --git a/proxmox-ve-config/src/host/utils.rs
> b/proxmox-ve-config/src/host/utils.rs
> new file mode 100644
> index 000..1636f95
> --- /dev/null
> +++ b/proxmox-ve-config/src/host/utils.rs
> @@ -0,0 +1,97 @@
> +use std::net::{IpAddr, ToSocketAddrs};
> +use std::sync::OnceLock;
> +
> +use crate::firewall::types::Cidr;
> +
> +use nix::sys::socket::{AddressFamily, SockaddrLike};
> +
> +pub fn hostname() -> &'static str {
> +static HOSTNAME: OnceLock = OnceLock::new();
> +
> +// We should rather use get_or_try_init to avoid needing to panic
> +// but it is currently experimental
> +HOSTNAME.get_or_init(|| {
> +use nix::libc::{c_char, gethostname, sysconf, _SC_HOST_NAME_MAX};
> +use std::ffi::CStr;
> +
> +let max_len = unsafe { sysconf(_SC_HOST_NAME_MAX) } as usize + 1;
> +let mut buffer = vec![0; max_len];
> +
> +let ret = unsafe { gethostname(buffer.as_mut_ptr() as *mut c_char,
> buffer.len()) };
> +
> +if ret != 0 {
> +// failing to get the hostname means something is *really* off
> +panic!("gethostname failed with returncode {ret}");
> +}
> +
> +let c_str = CStr::from_bytes_until_nul(&buffer).expect("buffer
> contains a NUL byte");
> +
> +String::from_utf8_lossy(c_str.to_bytes()).to_string()
> +})
IMO the closures here and below could later be put into something like
proxmox-sys (or similar) as freestanding functions without static data
and then called here - but this is fine as it is; just an idea!
> +}
> +
> +pub fn host_ips() -> &'static [IpAddr] {
> +static IP_ADDRESSES: OnceLock> = OnceLock::new();
> +
> +// We should rather use get_or_try_init to avoid needing to panic
> +// but it is currently experimental
> +IP_ADDRESSES.get_or_init(|| {
> +let hostname = hostname();
> +
> +format!("{hostname}:0")
> +.to_socket_addrs()
> +.expect("local hostname is resolvable")
> +.map(|addr| addr.ip())
> +.collect()
> +})
^ Here as well.
> +}
> +
> +pub fn network_interface_cidrs() -> &'static [Cidr] {
> +static INTERFACES: OnceLock> = OnceLock::new();
> +
> +// We should rather use get_or_try_init to avoid needing to panic
> +// but it is currently experimental
> +INTERFACES.get_or_init(|| {
> +use nix::ifaddrs::getifaddrs;
> +
> +let mut cidrs = Vec::new();
> +
> +let interfaces = getifaddrs().expect("should be able to query
> network interfaces");
> +
> +for interface in interfaces {
> +if let (Some(address), Some(netmask)) = (interface.address,
> interface.netmask) {
> +match (address.family(), netmask.family()) {
> +(Some(AddressFamily::Inet), Some(AddressFamily::Inet))
> => {
> +let address = address.as_sockaddr_in().expect("is an
> IPv4 address").ip();
> +
> +let netmask = netmask
> +.as_sockaddr_in()
> +.expect("is an IPv4 address")
> +.ip()
> +.count_ones()
> +.try_into()
> +.expect("count_ones of u32 is < u8_max");
> +
> +cidrs.push(Cidr::new_v4(address,
> netmask).expect("netmask is valid"));
> +}
> +
Re: [pve-devel] [PATCH proxmox-firewall 13/37] config: firewall: add host specific config + option types
On Tue Apr 2, 2024 at 7:16 PM CEST, Stefan Hanreich wrote:
> Co-authored-by: Wolfgang Bumiller
> Signed-off-by: Stefan Hanreich
> ---
> proxmox-ve-config/src/firewall/host.rs | 309 +
> proxmox-ve-config/src/firewall/mod.rs | 1 +
> 2 files changed, 310 insertions(+)
> create mode 100644 proxmox-ve-config/src/firewall/host.rs
>
> diff --git a/proxmox-ve-config/src/firewall/host.rs
> b/proxmox-ve-config/src/firewall/host.rs
> new file mode 100644
> index 000..3e47bfa
> --- /dev/null
> +++ b/proxmox-ve-config/src/firewall/host.rs
> @@ -0,0 +1,309 @@
> +use std::io;
> +use std::net::IpAddr;
> +
> +use anyhow::{bail, Error};
> +use serde::Deserialize;
> +
> +use crate::host::utils::{host_ips, hostname, network_interface_cidrs};
> +
> +use crate::firewall::parse;
> +use crate::firewall::types::log::LogLevel;
> +use crate::firewall::types::rule::Direction;
> +use crate::firewall::types::{Alias, Cidr, Rule};
> +
> +#[derive(Debug, Default, Deserialize)]
> +#[cfg_attr(test, derive(Eq, PartialEq))]
> +pub struct Options {
> +#[serde(default, with = "parse::serde_option_bool")]
> +enable: Option,
> +
> +#[serde(default, with = "parse::serde_option_bool")]
> +nftables: Option,
> +
> +log_level_in: Option,
> +log_level_out: Option,
> +
> +#[serde(default, with = "parse::serde_option_bool")]
> +log_nf_conntrack: Option,
> +#[serde(default, with = "parse::serde_option_bool")]
> +ndp: Option,
> +
> +#[serde(default, with = "parse::serde_option_bool")]
> +nf_conntrack_allow_invalid: Option,
> +
> +#[serde(default, with = "parse::serde_option_conntrack_helpers")]
> +nf_conntrack_helpers: Option>,
> +
> +#[serde(default, with = "parse::serde_option_number")]
> +nf_conntrack_max: Option,
> +#[serde(default, with = "parse::serde_option_number")]
> +nf_conntrack_tcp_timeout_established: Option,
> +#[serde(default, with = "parse::serde_option_number")]
> +nf_conntrack_tcp_timeout_syn_recv: Option,
> +
> +#[serde(default, with = "parse::serde_option_bool")]
> +nosmurfs: Option,
> +
> +#[serde(default, with = "parse::serde_option_bool")]
> +protection_synflood: Option,
> +#[serde(default, with = "parse::serde_option_number")]
> +protection_synflood_burst: Option,
> +#[serde(default, with = "parse::serde_option_number")]
> +protection_synflood_rate: Option,
> +
> +smurf_log_level: Option,
> +tcp_flags_log_level: Option,
> +
> +#[serde(default, with = "parse::serde_option_bool")]
> +tcpflags: Option,
> +}
> +
> +#[derive(Debug, Default)]
> +pub struct Config {
> +pub(crate) config: super::common::Config,
> +}
> +
> +impl Config {
> +pub fn new() -> Self {
> +Self {
> +config: Default::default(),
> +}
> +}
> +
> +pub fn parse(input: R) -> Result {
> +let config = super::common::Config::parse(input,
> &Default::default())?;
> +
> +if !config.groups.is_empty() {
> +bail!("host firewall config cannot declare groups");
> +}
> +
> +if !config.aliases.is_empty() {
> +bail!("host firewall config cannot declare aliases");
> +}
> +
> +if !config.ipsets.is_empty() {
> +bail!("host firewall config cannot declare ipsets");
> +}
> +
> +Ok(Self { config })
> +}
> +
> +pub fn rules(&self) -> &[Rule] {
> +&self.config.rules
> +}
> +
> +pub fn management_ips() -> Result, Error> {
> +let mut management_cidrs = Vec::new();
> +
> +for host_ip in host_ips() {
> +for network_interface_cidr in network_interface_cidrs() {
> +match (host_ip, network_interface_cidr) {
> +(IpAddr::V4(ip), Cidr::Ipv4(cidr)) => {
> +if cidr.contains_address(ip) {
> +
> management_cidrs.push(network_interface_cidr.clone());
> +}
> +}
> +(IpAddr::V6(ip), Cidr::Ipv6(cidr)) => {
> +if cidr.contains_address(ip) {
> +
> management_cidrs.push(network_interface_cidr.clone());
> +}
> +}
> +_ => continue,
> +};
> +}
> +}
> +
> +Ok(management_cidrs)
> +}
> +
> +pub fn hostname() -> &'static str {
> +hostname()
> +}
> +
> +pub fn get_alias(&self, name: &str) -> Option<&Alias> {
> +self.config.alias(name)
> +}
> +
> +pub fn is_enabled(&self) -> bool {
> +self.config.options.enable.unwrap_or(true)
> +}
> +
> +pub fn nftables(&self) -> bool {
> +self.config.options.nftables.unwrap_or(false)
> +}
> +
> +pub fn allow_ndp(&self) -> bool {
> +self.config.options.ndp.unwrap_or(true)
> +}
> +
> +pub fn block_smurfs(&self) -> bool {
> +self.conf
Re: [pve-devel] [PATCH proxmox-firewall 09/37] config: firewall: add types for rules
On Tue Apr 2, 2024 at 7:16 PM CEST, Stefan Hanreich wrote:
> Additionally we implement FromStr for all rule types and parts, which
> can be used for parsing firewall config rules. Initial rule parsing
> works by parsing the different options into a HashMap and only then
> de-serializing a struct from the parsed options.
>
> This intermediate step makes rule parsing a lot easier, since we can
> reuse the deserialization logic from serde. Also, we can split the
> parsing/deserialization logic from the validation logic.
>
> Co-authored-by: Wolfgang Bumiller
> Signed-off-by: Stefan Hanreich
> ---
> proxmox-ve-config/src/firewall/parse.rs | 185
> proxmox-ve-config/src/firewall/types/mod.rs | 3 +
> proxmox-ve-config/src/firewall/types/rule.rs | 412
> .../src/firewall/types/rule_match.rs | 953 ++
> 4 files changed, 1553 insertions(+)
> create mode 100644 proxmox-ve-config/src/firewall/types/rule.rs
> create mode 100644 proxmox-ve-config/src/firewall/types/rule_match.rs
>
> diff --git a/proxmox-ve-config/src/firewall/parse.rs
> b/proxmox-ve-config/src/firewall/parse.rs
> index 669623b..227e045 100644
> --- a/proxmox-ve-config/src/firewall/parse.rs
> +++ b/proxmox-ve-config/src/firewall/parse.rs
> @@ -1,3 +1,5 @@
> +use std::fmt;
> +
> use anyhow::{bail, format_err, Error};
>
> /// Parses out a "name" which can be alphanumeric and include dashes.
> @@ -78,3 +80,186 @@ pub fn parse_bool(value: &str) -> Result {
> },
> )
> }
> +
> +/// `&str` deserializer which also accepts an `Option`.
> +///
> +/// Serde's `StringDeserializer` does not.
> +#[derive(Clone, Copy, Debug)]
> +pub struct SomeStrDeserializer<'a, E>(serde::de::value::StrDeserializer<'a,
> E>);
> +
> +impl<'de, 'a, E> serde::de::Deserializer<'de> for SomeStrDeserializer<'a, E>
> +where
> +E: serde::de::Error,
> +{
> +type Error = E;
> +
> +fn deserialize_any(self, visitor: V) -> Result
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +self.0.deserialize_any(visitor)
> +}
> +
> +fn deserialize_option(self, visitor: V) -> Result Self::Error>
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +visitor.visit_some(self.0)
> +}
> +
> +fn deserialize_str(self, visitor: V) -> Result
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +self.0.deserialize_str(visitor)
> +}
> +
> +fn deserialize_string(self, visitor: V) -> Result Self::Error>
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +self.0.deserialize_string(visitor)
> +}
> +
> +fn deserialize_enum(
> +self,
> +_name: &str,
> +_variants: &'static [&'static str],
> +visitor: V,
> +) -> Result
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +visitor.visit_enum(self.0)
> +}
> +
> +serde::forward_to_deserialize_any! {
> +bool i8 i16 i32 i64 i128 u8 u16 u32 u64 u128 f32 f64 char
> +bytes byte_buf unit unit_struct newtype_struct seq tuple
> +tuple_struct map struct identifier ignored_any
> +}
> +}
> +
> +/// `&str` wrapper which implements `IntoDeserializer` via
> `SomeStrDeserializer`.
> +#[derive(Clone, Debug)]
> +pub struct SomeStr<'a>(pub &'a str);
> +
> +impl<'a> From<&'a str> for SomeStr<'a> {
> +fn from(s: &'a str) -> Self {
> +Self(s)
> +}
> +}
> +
> +impl<'de, 'a, E> serde::de::IntoDeserializer<'de, E> for SomeStr<'a>
> +where
> +E: serde::de::Error,
> +{
> +type Deserializer = SomeStrDeserializer<'a, E>;
> +
> +fn into_deserializer(self) -> Self::Deserializer {
> +SomeStrDeserializer(self.0.into_deserializer())
> +}
> +}
> +
> +/// `String` deserializer which also accepts an `Option`.
> +///
> +/// Serde's `StringDeserializer` does not.
> +#[derive(Clone, Debug)]
> +pub struct
> SomeStringDeserializer(serde::de::value::StringDeserializer);
> +
> +impl<'de, E> serde::de::Deserializer<'de> for SomeStringDeserializer
> +where
> +E: serde::de::Error,
> +{
> +type Error = E;
> +
> +fn deserialize_any(self, visitor: V) -> Result
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +self.0.deserialize_any(visitor)
> +}
> +
> +fn deserialize_option(self, visitor: V) -> Result Self::Error>
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +visitor.visit_some(self.0)
> +}
> +
> +fn deserialize_str(self, visitor: V) -> Result
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +self.0.deserialize_str(visitor)
> +}
> +
> +fn deserialize_string(self, visitor: V) -> Result Self::Error>
> +where
> +V: serde::de::Visitor<'de>,
> +{
> +self.0.deserialize_string(visitor)
> +}
> +
> +fn deserialize_enum(
> +self,
> +_name: &str,
> +_variants: &'static [&'static str],
> +visitor: V,
> +) -> Result
> +where
> +V: ser
Re: [pve-devel] [PATCH proxmox-firewall 21/37] nftables: statement: add types
On Tue Apr 2, 2024 at 7:16 PM CEST, Stefan Hanreich wrote:
> Adds an enum containing most of the statements defined in the
> nftables-json schema [1].
>
> [1]
> https://manpages.debian.org/bookworm/libnftables1/libnftables-json.5.en.html#STATEMENTS
>
> Co-authored-by: Wolfgang Bumiller
> Signed-off-by: Stefan Hanreich
> ---
> proxmox-nftables/Cargo.toml | 1 +
> proxmox-nftables/src/lib.rs | 2 +
> proxmox-nftables/src/statement.rs | 321 ++
> proxmox-nftables/src/types.rs | 17 ++
> 4 files changed, 341 insertions(+)
> create mode 100644 proxmox-nftables/src/statement.rs
>
> diff --git a/proxmox-nftables/Cargo.toml b/proxmox-nftables/Cargo.toml
> index 7e607e8..153716d 100644
> --- a/proxmox-nftables/Cargo.toml
> +++ b/proxmox-nftables/Cargo.toml
> @@ -15,6 +15,7 @@ config-ext = ["dep:proxmox-ve-config"]
>
> [dependencies]
> log = "0.4"
> +anyhow = "1"
>
> serde = { version = "1", features = [ "derive" ] }
> serde_json = "1"
> diff --git a/proxmox-nftables/src/lib.rs b/proxmox-nftables/src/lib.rs
> index 712858b..40f6bab 100644
> --- a/proxmox-nftables/src/lib.rs
> +++ b/proxmox-nftables/src/lib.rs
> @@ -1,5 +1,7 @@
> pub mod expression;
> pub mod helper;
> +pub mod statement;
> pub mod types;
>
> pub use expression::Expression;
> +pub use statement::Statement;
> diff --git a/proxmox-nftables/src/statement.rs
> b/proxmox-nftables/src/statement.rs
> new file mode 100644
> index 000..e569f33
> --- /dev/null
> +++ b/proxmox-nftables/src/statement.rs
> @@ -0,0 +1,321 @@
> +use anyhow::{bail, Error};
Hmm, you don't use either here - you sure you didn't mean to introduce
`anyhow` later?
> +use serde::{Deserialize, Serialize};
> +
> +use crate::expression::Meta;
> +use crate::helper::{NfVec, Null};
> +use crate::types::{RateTimescale, RateUnit, Verdict};
> +use crate::Expression;
> +
> +#[derive(Clone, Debug, Deserialize, Serialize)]
> +#[serde(rename_all = "lowercase")]
> +pub enum Statement {
> +Match(Match),
> +Mangle(Mangle),
> +Limit(Limit),
> +Notrack(Null),
> +Reject(Reject),
> +Set(Set),
> +Log(Log),
> +#[serde(rename = "ct helper")]
> +CtHelper(String),
> +Vmap(Vmap),
> +Comment(String),
> +
> +#[serde(untagged)]
> +Verdict(Verdict),
> +}
> +
> +impl Statement {
> +pub const fn make_accept() -> Self {
> +Statement::Verdict(Verdict::Accept(Null))
> +}
> +
> +pub const fn make_drop() -> Self {
> +Statement::Verdict(Verdict::Drop(Null))
> +}
> +
> +pub const fn make_return() -> Self {
> +Statement::Verdict(Verdict::Return(Null))
> +}
> +
> +pub const fn make_continue() -> Self {
> +Statement::Verdict(Verdict::Continue(Null))
> +}
> +
> +pub fn jump(target: impl Into) -> Self {
> +Statement::Verdict(Verdict::Jump {
> +target: target.into(),
> +})
> +}
> +
> +pub fn goto(target: impl Into) -> Self {
> +Statement::Verdict(Verdict::Goto {
> +target: target.into(),
> +})
> +}
> +}
> +
> +impl From for Statement {
> +#[inline]
> +fn from(m: Match) -> Statement {
> +Statement::Match(m)
> +}
> +}
> +
> +impl From for Statement {
> +#[inline]
> +fn from(m: Mangle) -> Statement {
> +Statement::Mangle(m)
> +}
> +}
> +
> +impl From for Statement {
> +#[inline]
> +fn from(m: Reject) -> Statement {
> +Statement::Reject(m)
> +}
> +}
> +
> +impl From for Statement {
> +#[inline]
> +fn from(m: Set) -> Statement {
> +Statement::Set(m)
> +}
> +}
> +
> +impl From for Statement {
> +#[inline]
> +fn from(m: Vmap) -> Statement {
> +Statement::Vmap(m)
> +}
> +}
> +
> +impl From for Statement {
> +#[inline]
> +fn from(log: Log) -> Statement {
> +Statement::Log(log)
> +}
> +}
> +
> +impl> From for Statement {
> +#[inline]
> +fn from(limit: T) -> Statement {
> +Statement::Limit(limit.into())
> +}
> +}
> +
> +#[derive(Clone, Debug, Deserialize, Serialize)]
> +#[serde(rename_all = "lowercase")]
> +pub enum RejectType {
> +#[serde(rename = "tcp reset")]
> +TcpRst,
> +IcmpX,
> +Icmp,
> +IcmpV6,
> +}
> +
> +#[derive(Clone, Debug, Default, Deserialize, Serialize)]
> +pub struct Reject {
> +#[serde(rename = "type", skip_serializing_if = "Option::is_none")]
> +ty: Option,
> +#[serde(skip_serializing_if = "Option::is_none")]
> +expr: Option,
> +}
> +
> +#[derive(Clone, Debug, Default, Deserialize, Serialize)]
> +#[serde(rename_all = "kebab-case")]
> +pub struct Log {
> +#[serde(skip_serializing_if = "Option::is_none")]
> +prefix: Option,
> +
> +#[serde(skip_serializing_if = "Option::is_none")]
> +group: Option,
> +
> +#[serde(skip_serializing_if = "Option::is_none")]
> +snaplen: Option,
> +
> +#[serde(skip_serializing_if = "Option::is_none")]
> +queue_threshold: Option,
> +
> +
[pve-devel] [PATCH pve-kernel] revert cifs backport to 6.1 added between 6.5.13-1 and 6.5.13-2
copying files within a cifs-share currently result in the following trace: ``` [ 495.388739] BUG: unable to handle page fault for address: fffe [ 495.388744] #PF: supervisor read access in kernel mode [ 495.388746] #PF: error_code(0x) - not-present page [ 495.388747] PGD 172c3f067 P4D 172c3f067 PUD 172c41067 PMD 0 [ 495.388752] Oops: [#2] PREEMPT SMP NOPTI [ 495.388754] CPU: 1 PID: 3894 Comm: cp Tainted: G D 6.5.0-32-generic #32-Ubuntu [ 495.388756] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 4.2023.08-4 02/15/2024 [ 495.388758] RIP: 0010:cifs_flush_folio+0x41/0xf0 [cifs] ... ``` a quick check identified proxmox-kernel-6.5.13-2 as the first affected version, and `2dc07a11e269bfbe5589e99b60cdbae0118be979` as likely source of the issue. The commit adapts the changes from `7b2404a886f8b91250c31855d287e632123e1746` to work with the code in kernel 6.1. This is not needed as the relevant changes were made in 6.4 and are already part of the 6.5 tree - `66dabbb65d673aef40dd17bf62c042be8f6d4a4b` reverting the commit fixes copying files within a samba share. Tested/reproduced with: * a VM with the kernel as cifs-client * one very crude samba-share allowing guest-write access on a Debian bookworm host * as well as a share using cifscreds + multiuser (`mount.cifs(8)`) * mounting the share, copying any file from one directory to another on the same share (with `cp` and Thunar and Nautilus). Reported to Ubuntu upstream at [1]. [0] https://lore.kernel.org/linux-mm/zzhrpnj3zxmr8...@eldamar.lan/ [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2055002 Reported-by: Daniela Häsler Signed-off-by: Stoiko Ivanov --- ...flushing-folio-regression-for-6.1-ba.patch | 23 +++ 1 file changed, 23 insertions(+) create mode 100644 patches/kernel/0014-Revert-cifs-fix-flushing-folio-regression-for-6.1-ba.patch diff --git a/patches/kernel/0014-Revert-cifs-fix-flushing-folio-regression-for-6.1-ba.patch b/patches/kernel/0014-Revert-cifs-fix-flushing-folio-regression-for-6.1-ba.patch new file mode 100644 index ..e033b68ac69f --- /dev/null +++ b/patches/kernel/0014-Revert-cifs-fix-flushing-folio-regression-for-6.1-ba.patch @@ -0,0 +1,23 @@ +From Mon Sep 17 00:00:00 2001 +From: Stoiko Ivanov +Date: Wed, 3 Apr 2024 10:29:59 +0200 +Subject: [PATCH] Revert "cifs: fix flushing folio regression for 6.1 backport" + +This reverts commit 2dc07a11e269bfbe5589e99b60cdbae0118be979. +--- + fs/smb/client/cifsfs.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c +index 55a6d0296ec82..82313b2534631 100644 +--- a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c +@@ -1245,7 +1245,7 @@ static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, lo + int rc = 0; + + folio = filemap_get_folio(inode->i_mapping, index); +- if (!folio) ++ if (IS_ERR(folio)) + return 0; + + size = folio_size(folio); -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 pve-network 0/9] SDN: Testing VNets as a blackbox.
On Tue Apr 2, 2024 at 6:07 PM CEST, Stefan Lendl wrote: > This add several tests for SDN VNets. > State setup as well as testing results is done only via the API to test on the > API boundaries and not against the internal state. Internal state and config > files are mocked to avoid requiring access to system files or pmxcfs. > > The first 7 commits extract various functions to allow mocking them in the > tests. The tests are then added as the test_vnets_blackbox test. > The last commit removes the old vnets tests which are not working anyway. > > Tests validate the events of a nic joining a Vnet or a nic staring on a vnet > with different subnet configurations. Further descriptions in the commit. > > *Several of the tests fail due to known bugs and limitations.* > These bugs where discussed off-list with @s.hanreich and due to be fixed with > the next set of SDN patches. > Applying the tests first would allow fixing them with already existing > tests. > > Differences v1 -> v2: > * Add tests that expect a failure when no IP can be allocated > * Removed commented out debug stuff Just to be sure I've looked over the individual patches again - LGTM! I've got no complaints at all. Nice work! I'll leave the remaining things regarding the failing tests to you and @s.hanreich, as that's your domain ;) > > Stefan Lendl (9): > sdn: extract function that reads datacenter config > dnsmasq: extract function to systemctl command. > dnsmasq: extract function that generates the ethers file path > dnsmasq: extract function that updates dnsmasq lease via dbus > controllers: extract function that reads network intreaces config > evpn: extract function that reads frr config. > api: extract function that creates the sdn directory. > tests: test VNets functionality as a blackbox > tests: remove old Vnets tests > > src/PVE/API2/Network/SDN/Zones.pm | 6 +- > src/PVE/Network/SDN/Controllers.pm| 16 +- > src/PVE/Network/SDN/Controllers/EvpnPlugin.pm | 10 +- > src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 47 +- > src/PVE/Network/SDN/Zones/EvpnPlugin.pm | 3 +- > src/PVE/Network/SDN/Zones/Plugin.pm | 5 + > src/PVE/Network/SDN/Zones/SimplePlugin.pm | 2 +- > src/test/Makefile | 5 +- > src/test/run_test_vnets.pl| 343 --- > src/test/run_test_vnets_blackbox.pl | 859 ++ > 10 files changed, 925 insertions(+), 371 deletions(-) > delete mode 100755 src/test/run_test_vnets.pl > create mode 100755 src/test/run_test_vnets_blackbox.pl ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
--- Begin Message --- > Maybe it is time to disable dynamic mac-learning by default ? > The code is already here and works fine. > > AFAIK, other hypervisor like vmware disable port flooding by default > with static mac registration too. >>Might be a good idea, although it still wouldn't solve the problem - >>sadly (since we're still not allowed to do REJECT then). maybe revert the kernel patch ? ^_^ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/bridge/netfilter/nft_reject_bridge.c?h=v6.8.2&id=127917c29a432c3b798e014a1714e9c1af0f87fe Or Improve it for upstream, something like: if !bridge_unicast_flooding && !bridge_mac_learning && proto = tcp|udp allow_use_of_reject as the original commit message seem to be about unicast flood " If we allow this to be used from forward or any other later bridge hook, if the frame is flooded to several ports, we'll end up sending several reject packets, " ___ pve-devel mailing list pve-devel@lists.proxmox.com https://antiphishing.vadesecure.com/v4?f=dVpnOERZb0JKOFlaRnBNeQ- aJAXZb5aW6JXm5NyXq0ZSryyNaYxsZDLB8WDV0q4oZylZ86zxfmQyzg5dawW4cw&i=TG56O W16ck5wUlFINGEzQ79EVPOILSGYD2XAUbTQrkI&k=1ZtS&r=enJEWGxReW5qbm5MS3pxTW8 Kub8XGodVNRkE_1iQQaZcsg_WcpdPfj8fEnEUbIAG&s=df68f05c7c9a0ea625e65001c10 eadba11343149ec52826a395f84870d55994a&u=https%3A%2F%2Flists.proxmox.com %2Fcgi-bin%2Fmailman%2Flistinfo%2Fpve-devel --- End Message --- ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
On 4/3/24 14:03, DERUMIER, Alexandre via pve-devel wrote: > maybe revert the kernel patch ? ^_^ > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/bridge/netfilter/nft_reject_bridge.c?h=v6.8.2&id=127917c29a432c3b798e014a1714e9c1af0f87fe I also thought about it shortly. If we can ensure that certain conditions are met that might be an option. We would have to think about broadcast/multicast traffic like ARP / DHCP I would assume. It seems a bit drastic from my POV, which is why dropped that thought. > Or Improve it for upstream, something like: > > if !bridge_unicast_flooding && !bridge_mac_learning && proto = tcp|udp > allow_use_of_reject that might be a possibility, although I'm not sure that information about the bridge is available in the netfilter modules. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
also as a short fyi, since I forgot to mention it in my cover letter: I've refrained from adding stuff like flowtables and broute for now - but it is certainly something I want to add in future revisions. For the initial POC I wanted to stay as basic as possible and create a 1:1 replacement without any fancy bells and whistles. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
--- Begin Message --- Message initial De: Stefan Hanreich Répondre à: Proxmox VE development discussion À: pve-devel@lists.proxmox.com Objet: Re: [pve-devel] [RFC container/firewall/manager/proxmox- firewall/qemu-server 00/37] proxmox firewall nftables implementation Date: 03/04/2024 14:25:40 On 4/3/24 14:03, DERUMIER, Alexandre via pve-devel wrote: > maybe revert the kernel patch ? ^_^ > https://antiphishing.vadesecure.com/v4?f=YVdEbUdjdUhGSnlic1ZwZTZs5NC0 > IK5UpoMm-JbYmH0g8SvUq6T2pULKyCWNdAtigmuEY2RK_MUtsxeEJS- > hxg&i=VG1PWDJGYXFXcTREa3RZRfomnJARQnrbyjpk2iZcdNI&k=BnAq&r=M1hxaWZ5bn > NuVExjSWtSa9ErN2N5EUBGcCr8vO1Apj6WrYRZZdbucoU7ZCJIs1cd&s=3ec641e9b97c > 0cd606bd785a4f86cf529a63c00affecb597ebc3fa3e7f6b8764&u=https%3A%2F%2F > git.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.gi > t%2Fcommit%2Fnet%2Fbridge%2Fnetfilter%2Fnft_reject_bridge.c%3Fh%3Dv6. > 8.2%26id%3D127917c29a432c3b798e014a1714e9c1af0f87fe >>I also thought about it shortly. If we can ensure that certain >>conditions are met that might be an option. We would have to think >>about >>broadcast/multicast traffic like ARP / DHCP I would assume. It seems >>a >>bit drastic from my POV, which is why dropped that thought. I think you can just use DROP for this kind of traffic, as anyway, you don't expect to receive a response like tcp-reset or icmp port unreachable. only udp,tcp,icmp (unicast traffic) should be interesting (and can be protected from unicast_flood) > Or Improve it for upstream, something like: > > if !bridge_unicast_flooding && !bridge_mac_learning && proto = > tcp|udp > allow_use_of_reject >>that might be a possibility, although I'm not sure that information >>about the bridge is available in the netfilter modules. Yes, I'm not sure that it's possible to have this kind of information. Maybe simply adding a sysctl flag to allow|disallow usage of reject in forward/postrouting. Like this, the userland software can enabled it if it known that's it's safe. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://antiphishing.vadesecure.com/v4?f=YVdEbUdjdUhGSnlic1ZwZTZs5NC0IK 5UpoMm-JbYmH0g8SvUq6T2pULKyCWNdAtigmuEY2RK_MUtsxeEJS- hxg&i=VG1PWDJGYXFXcTREa3RZRfomnJARQnrbyjpk2iZcdNI&k=BnAq&r=M1hxaWZ5bnNu VExjSWtSa9ErN2N5EUBGcCr8vO1Apj6WrYRZZdbucoU7ZCJIs1cd&s=18886ae882494937 dd226c13263c6706f72f922dea16c781a962348d3ddbfc6a&u=https%3A%2F%2Flists. proxmox.com%2Fcgi-bin%2Fmailman%2Flistinfo%2Fpve-devel --- End Message --- ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
On 4/3/24 15:04, DERUMIER, Alexandre via pve-devel wrote: > I think you can just use DROP for this kind of traffic, as anyway, you > don't expect to receive a response like tcp-reset or icmp port > unreachable. Yes, of course, replied too quickly without thinking twice... ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 33/37] firewall: add files for debian packaging
just looked at the packaging, mostly related to clean building, but not
only.
On April 2, 2024 7:16 pm, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich
> ---
> Makefile| 93 +
> debian/changelog| 5 ++
> debian/control | 31 +++
> debian/copyright| 16 ++
> debian/proxmox-firewall.service | 16 ++
> debian/proxmox-firewall.timer | 11
> debian/rules| 14 +
> debian/source/format| 1 +
> defines.mk | 13 +
> 9 files changed, 200 insertions(+)
> create mode 100644 Makefile
> create mode 100644 debian/changelog
> create mode 100644 debian/control
> create mode 100644 debian/copyright
> create mode 100644 debian/proxmox-firewall.service
> create mode 100644 debian/proxmox-firewall.timer
> create mode 100644 debian/rules
> create mode 100644 debian/source/format
> create mode 100644 defines.mk
>
> diff --git a/Makefile b/Makefile
> new file mode 100644
> index 000..984c318
> --- /dev/null
> +++ b/Makefile
> @@ -0,0 +1,93 @@
> +include /usr/share/dpkg/pkg-info.mk
> +include /usr/share/dpkg/architecture.mk
> +include defines.mk
> +
> +PACKAGE=proxmox-firewall
> +BUILDDIR ?= $(PACKAGE)-$(DEB_VERSION_UPSTREAM)
> +
> +
> +DEB=$(PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION)_$(DEB_HOST_ARCH).deb
> +DBG_DEB=$(PACKAGE)-dbgsym_$(DEB_VERSION_UPSTREAM_REVISION)_$(DEB_HOST_ARCH).deb
> +DSC=rust-$(PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION).dsc
this doesn't match d/control ;)
> +
> +DEBS = $(DEB) $(DBG_DEB)
> +
> +ifeq ($(BUILD_MODE), release)
you need to set/export this in d/rules, else the package will contain a
debug build..
> +CARGO_BUILD_ARGS += --release
> +COMPILEDIR := target/release
> +else
> +COMPILEDIR := target/debug
> +endif
> +
> +USR_BIN := \
> + proxmox-firewall
> +
> +COMPILED_BINS := \
> + $(addprefix $(COMPILEDIR)/,$(USR_BIN))
> +
> +all: cargo-build
> +
> +.PHONY: cargo-build
> +cargo-build:
> + cargo build $(CARGO_BUILD_ARGS)
> +
> +$(COMPILED_BINS): cargo-build
> +
> +install: $(COMPILED_BINS)
> + install -dm755 $(DESTDIR)$(SBINDIR)
> + $(foreach i,$(USR_BIN), \
> + install -m755 $(COMPILEDIR)/$(i) $(DESTDIR)$(SBINDIR)/ ;)
I am not sure this (and all the helper stuff above that only exists for
it) make much sense. maybe we could simply get `$(CARGO) install` to
work here, and then let the packging put it into /usr/sbin ?
> +
> +update-dcontrol: #$(BUILDDIR)
> + debcargo package \
> + --config debian/debcargo.toml \
> + --changelog-ready \
> + --no-overlay-write-back \
> + --directory $(BUILDDIR) \
> + $(PACKAGE) \
> + $(shell dpkg-parsechangelog -l debian/changelog -SVersion | sed -e
> 's/-.*//')
> + cat $(BUILDDIR)/debian/control debian/control.extra > debian/control
> + wrap-and-sort -t -k -f debian/control
this doesn't work because there is no debian/debcargo.toml and also no
debian/control.extra ;) since debcargo doesn't work with workspaces
(yet), it probably doesn't make sense to leave this in.. one just has to
remember to update d/control when updating any of the Cargo.toml files
w.r.t. dependencies or features.
> +
> +.PHONY: build
> +build: $(BUILDDIR)
> +$(BUILDDIR):
> + rm -rf $@ $@.tmp; mkdir $@.tmp
> + cp -a proxmox-firewall proxmox-nftables proxmox-ve-config debian
> Cargo.toml Makefile defines.mk $@.tmp/
this is missing the .cargo dir, which doesn't matter (much) for building
directly via `make deb` (since cargo will look in parent dirs), but
completely breaks for `make dsc` or `make sbuild`. but I suggest
handling that in d/rules (see comment there)
> + mv $@.tmp $@
> +
> +.PHONY: deb
> +deb: $(DEB)
> +$(HELPER_DEB) $(DBG_DEB) $(HELPER_DBG_DEB) $(DOC_DEB): $(DEB)
> +$(DEB): $(BUILDDIR)
> + cd $(BUILDDIR); dpkg-buildpackage -b -us -uc --no-pre-clean
> + lintian $(DEB) $(DOC_DEB) $(HELPER_DEB)
> +
> +.PHONY: test
> +test:
> + cargo test
> +
> +.PHONY: dsc
> +dsc:
> + rm -rf $(BUILDDIR) $(DSC)
> + $(MAKE) $(DSC)
> + lintian $(DSC)
> +$(DSC): $(BUILDDIR)
> + cd $(BUILDDIR); dpkg-buildpackage -S -us -uc -d -nc
> +
> +sbuild: $(DSC)
> + sbuild $<
> +
> +.PHONY: dinstall
> +dinstall: $(DEB)
> + dpkg -i $(DEB) $(DBG_DEB) $(DOC_DEB)
> +
> +.PHONY: distclean
> +distclean: clean
> +
> +.PHONY: clean
> +clean:
> + cargo clean
> + rm -f *.deb *.build *.buildinfo *.changes *.dsc rust-$(PACKAGE)*.tar*
> + rm -rf $(PACKAGE)-[0-9]*/
> + find . -name '*~' -exec rm {} ';'
> diff --git a/debian/changelog b/debian/changelog
> new file mode 100644
> index 000..7918ec9
> --- /dev/null
> +++ b/debian/changelog
> @@ -0,0 +1,5 @@
> +proxmox-firewall (0.1-1) UNRELEASED; urgency=medium
the `-1` and `debian/source/format` disagree (again, only a problem when
attempting building via sbuild)
> +
> + * Initial release.
> +
> + -- Stefan Hanreich Thu, 07 M
[pve-devel] [PATCH pve-storage 1/1] storage/plugin: implement ref-counting for disknames in get_next_vm_diskname
As Fabian has already mentioned here[0], there can be a race between two
parallel imports. More specifically, if both imports have --allow-rename
set and the desired name already exists, then it can happen that both
imports get the same name. The reason for this is that we currently only
check which names have already been assigned in the vm config and then
use the next free one, and do not check whether a running process has
already been assigned the same name. However, while writing and testing
the patch, I found that this is often not a problem, as
- in most cases the source VM config is locked and therefore only one
process can generate a new name (migrate, clone, move disk, update
config)
- a name must be specified and therefore no new name is generated (pvesm
alloc)
- the timeframe for the race is very short.
At the same time, it is possible that I have not considered all edge
cases and that there are other cases where the race can occur,
especially with regard to remote_migrations. You can provoke the problem
with two parallel imports on a host where local-lvm:vm-100-disk-0
already exists:
pvesm import local-lvm:vm-100-disk-0 raw+size --allow-rename 1
Now that I've looked into the problem a bit, I'm not sure this patch is
even necessary as it adds more complexity. So I wanted to ask for your
opinion, wether you think it makes sense to add this change or not.
The patch introduces a tmp file which stores the newly assigned disk
names and the pid of the process which requested the disk name. If a
second process is assigned the same name, it will see from the file that
the name has already been assigned to another process, and will take the
next available name. Reading and writing to the tmp file requires a lock
to prevent races.
[0] https://lists.proxmox.com/pipermail/pve-devel/2024-January/061526.html
Signed-off-by: Hannes Duerr
---
src/PVE/Storage/Plugin.pm | 99 ++-
1 file changed, 86 insertions(+), 13 deletions(-)
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 7456c8e..f76550a 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -10,8 +10,10 @@ use File::chdir;
use File::Path;
use File::Basename;
use File::stat qw();
+use File::Copy;
use PVE::Tools qw(run_command);
+use PVE::ProcFSTools;
use PVE::JSONSchema qw(get_standard_option register_standard_option);
use PVE::Cluster qw(cfs_register_file);
@@ -779,23 +781,94 @@ my $get_vm_disk_number = sub {
sub get_next_vm_diskname {
my ($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix) = @_;
-$fmt //= '';
-my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm';
-my $suffix = $add_fmt_suffix ? ".$fmt" : '';
+my $code = sub {
+ my $reserved_names_file = "/var/tmp/pve-reserved-volnames";
+ my $tmp_file = "/var/tmp/pve-reserved-volnames.tmp";
-my $disk_ids = {};
-foreach my $disk (@$disk_list) {
- my $disknum = $get_vm_disk_number->($disk, $scfg, $vmid, $suffix);
- $disk_ids->{$disknum} = 1 if defined($disknum);
-}
+ $fmt //= '';
+ my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm';
+ my $suffix = $add_fmt_suffix ? ".$fmt" : '';
+ my $disk_id;
+ my $disk_ids = {};
-for (my $i = 0; $i < $MAX_VOLUMES_PER_GUEST; $i++) {
- if (!$disk_ids->{$i}) {
- return "$prefix-$vmid-disk-$i$suffix";
+ foreach my $disk (@$disk_list) {
+ my $disknum = $get_vm_disk_number->($disk, $scfg, $vmid, $suffix);
+ $disk_ids->{$disknum} = 1 if defined($disknum);
}
-}
-die "unable to allocate an image name for VM $vmid in storage '$storeid'\n"
+ for (my $i = 0; $i < $MAX_VOLUMES_PER_GUEST; $i++) {
+ if (!$disk_ids->{$i}) {
+ $disk_id = $i;
+ last;
+ }
+ }
+
+ if (! -e $reserved_names_file) {
+ my $create_h = IO::File->new($reserved_names_file, "w") ||
+ die "can't open or create'$reserved_names_file' - $!\n";
+ print $create_h "$storeid $vmid $disk_id $$";
+ $create_h->close;
+
+ return "$prefix-$vmid-disk-$disk_id$suffix";
+ } else {
+ my $collision;
+ my $pid;
+
+ my $in_h = IO::File->new($reserved_names_file, "r") ||
+ die "can't open or create'$reserved_names_file' - $!\n";
+ my $out_h = IO::File->new($tmp_file, "w") ||
+ die "can't open or create'$tmp_file' - $!\n";
+
+ # remove entries when the process does not exist anymore
+ while (my $line = <$in_h>) {
+ ($pid) = $line =~ /$PVE::JSONSchema::PVE_ID \d+ \d+ (\d+)/;
+ next if (!defined $pid || !defined
PVE::ProcFSTools::check_process_running($pid));
+ print $out_h $line;
+ }
+
+ $in_h->close;
+ $out_h->close;
+ move($tmp_file, $reserved_names_file);
+
+ for (my $y = $d
Re: [pve-devel] GET /access/users/{userid} has parameter 'tokens' with 'additionalProperties' containing object definition
--- Begin Message ---
Hi,
Right, that makes a lot more sense! So the correct reading is more akin to
"an object that contains unknown keys, but the objects behind those keys
have a known schema"
Thank you for clarifying!
Kind regards,
Johannes
On Tue, Apr 2, 2024 at 10:00 AM Wolfgang Bumiller
wrote:
> On Tue, Apr 02, 2024 at 09:27:57AM +0200, Fabian Grünbichler wrote:
> >
> > > Jona Draaijer via pve-devel hat am
> 01.04.2024 22:00 CEST geschrieben:
> > > Hi,
> > >
> > > As per the title, that endpoint has an additionalProperties value that
> is
> > > not a bool, but rather an object definition. (It's defined in
> > > pve-access-control/src/PVE/API2/User.pm).
> > >
> > > As far as I can tell, all other 'additionalProperties' are bools. Does
> > > anyone know why this specific one is different, or if this difference
> is
> > > intentional?
> >
> > I think this was just an accident.
> >
> > > From the looks of it it seems like it's used as a "we need this
> standard
> > > option, but also have to make it optional". I am still quite new to
> perl,
> > > so I don't know if there is a way to do what was intended.
> >
> > My guess is the intent was to have
> >
> > tokens => get_standard_option('token-info', { optional => 1 }),
> >
> > instead, @Wolfgang?
>
> The `token-info` standard option defines the structure of a single
> token, which is a hash containing `expire`, `privsep` and `comment`.
> If we map `tokens` to that, then `tokens` would be exactly one token.
>
> The returned `tokens` value is a hash mapping token names to their info,
> like:
>
> "tokens" : {
> "first" : {
> "expire" : 0,
> "privsep" : 1
> },
> "second" : {
> "expire" : 0,
> "privsep" : 1
> }
> }
>
> So, each value inside the `tokens` object has the `token-info` schema.
>
> Most other API calls seem to return an array with the id merged into it
> as "tokenid", but this call did it this way from the beginning...
>
> But yes, we don't *usually* do this, but our JSON schema validator
> supported this and the API call was added like this originally, so
> declaring its schema this way was easier.
>
> You can see the definition in the JSON schema spec[1]. Note that `true`
> and `false` are not explicitly mentioned there, as these values are
> themselves considered schemas that "produce themselves as assertion
> results"[2].
>
> Also note that our schema isn't fully compliant with the spec
> (for instance we declare required vs optional fields differently), but
> at least feature-wise we do try to stick to keeping it a subset of it
> (with the occasional weirdness-extension to deal with legacy cruft such
> as how you can use the *value* of the *model* property as a *key* to
> declare the mac address in a network interface for a VM's network
> device... please don't :-) )
>
> [1]
> https://json-schema.org/draft/2020-12/json-schema-core#section-10.3.2.3
> [2] https://json-schema.org/draft/2020-12/json-schema-core#section-4.3.2
>
>
--- End Message ---
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
