Change the existing firewall rule generation logic to accomodate the changes in the config parser, so it can deal with the legacy ipset / alias names.
The lookup logic is the same as in pve-firewall: In guest context, check if it is contained in the guest config, otherwise check the cluster config. In cluster context, only check the cluster configuration. Because the rule generation logic now has to look up ipsets instead of blindly inserting rules with the ipset name, store the generated SDN ipsets in the firewall config as well, to avoid re-generating them for every lookup. This should also allow for better error reporting, in case ipsets or aliases are referenced that do not exist in the configuration. Signed-off-by: Stefan Hanreich <[email protected]> --- proxmox-firewall/src/config.rs | 93 ++++++++-- proxmox-firewall/src/firewall.rs | 11 +- proxmox-firewall/src/object.rs | 4 +- proxmox-firewall/src/rule.rs | 26 ++- proxmox-firewall/tests/input/cluster.fw | 2 + .../integration_tests__firewall.snap | 172 ++++++++++++++++++ 6 files changed, 273 insertions(+), 35 deletions(-) diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs index 8bd9f2a..aa8829f 100644 --- a/proxmox-firewall/src/config.rs +++ b/proxmox-firewall/src/config.rs @@ -11,8 +11,10 @@ use proxmox_ve_config::firewall::bridge::Config as BridgeConfig; use proxmox_ve_config::firewall::cluster::Config as ClusterConfig; use proxmox_ve_config::firewall::guest::Config as GuestConfig; use proxmox_ve_config::firewall::host::Config as HostConfig; -use proxmox_ve_config::firewall::types::alias::{Alias, AliasName, AliasScope}; +use proxmox_ve_config::firewall::types::alias::{Alias, AliasScope, RuleAliasName}; +use proxmox_ve_config::firewall::types::ipset::{IpsetScope, RuleIpsetName}; +use proxmox_ve_config::firewall::types::Ipset; use proxmox_ve_config::guest::types::Vmid; use proxmox_ve_config::guest::{GuestEntry, GuestMap}; use proxmox_ve_config::host::types::BridgeName; @@ -257,13 +259,28 @@ impl NftConfigLoader for PveNftConfigLoader { } } +pub struct FirewallSdnConfig { + _config: SdnConfig, + ipsets: BTreeMap<String, Ipset>, +} + +impl FirewallSdnConfig { + pub fn ipsets(&self) -> &BTreeMap<String, Ipset> { + &self.ipsets + } + + pub fn ipset(&self, name: &str) -> Option<&Ipset> { + self.ipsets.get(name) + } +} + pub struct FirewallConfig { cluster_config: ClusterConfig, host_config: HostConfig, guest_config: BTreeMap<Vmid, GuestConfig>, bridge_config: BTreeMap<BridgeName, BridgeConfig>, nft_config: BTreeMap<String, ListChain>, - sdn_config: Option<SdnConfig>, + sdn_config: Option<FirewallSdnConfig>, ipam_config: Option<Ipam>, interface_mapping: AltnameMapping, } @@ -325,11 +342,21 @@ impl FirewallConfig { pub fn parse_sdn( firewall_loader: &dyn FirewallConfigLoader, - ) -> Result<Option<SdnConfig>, Error> { + ) -> Result<Option<FirewallSdnConfig>, Error> { Ok(match firewall_loader.sdn_running_config()? { Some(data) => { let running_config: RunningConfig = serde_json::from_reader(data)?; - Some(SdnConfig::try_from(running_config)?) + let config = SdnConfig::try_from(running_config)?; + + let ipsets = config + .ipsets(None) + .map(|ipset| (ipset.name().name().to_string(), ipset)) + .collect(); + + Some(FirewallSdnConfig { + _config: config, + ipsets, + }) } _ => None, }) @@ -415,7 +442,7 @@ impl FirewallConfig { &self.nft_config } - pub fn sdn(&self) -> Option<&SdnConfig> { + pub fn sdn(&self) -> Option<&FirewallSdnConfig> { self.sdn_config.as_ref() } @@ -431,22 +458,54 @@ impl FirewallConfig { self.interface_mapping.get(iface_name).map(|x| x.as_str()) } - pub fn alias(&self, name: &AliasName, vmid: Option<Vmid>) -> Option<&Alias> { - log::trace!("getting alias {name:?}"); + fn guest_alias(&self, name: &str, vmid: Vmid) -> Option<&Alias> { + if let Some(guest_config) = self.guests().get(&vmid) { + return guest_config.alias(name); + } - match name.scope() { - AliasScope::Datacenter => self.cluster().alias(name.name()), - AliasScope::Guest => { - if let Some(vmid) = vmid { - if let Some(entry) = self.guests().get(&vmid) { - return entry.alias(name); - } + log::warn!("trying to get alias {name} for non-existing guest: #{vmid}"); + None + } - log::warn!("trying to get alias {name} for non-existing guest: #{vmid}"); + pub fn alias(&self, name: &RuleAliasName, vmid: Option<Vmid>) -> Option<&Alias> { + log::trace!("getting alias {name:?}"); + + match name { + RuleAliasName::Scoped(alias_name) => match alias_name.scope() { + AliasScope::Datacenter => self.cluster().alias(alias_name.name()), + AliasScope::Guest => { + vmid.and_then(|vmid| self.guest_alias(alias_name.name(), vmid)) } + }, + RuleAliasName::Legacy(legacy_alias_name) => vmid + .and_then(|vmid| self.guest_alias(legacy_alias_name.as_ref(), vmid)) + .or_else(|| self.cluster().alias(legacy_alias_name.as_ref())), + } + } - None - } + fn guest_ipset(&self, name: &str, vmid: Vmid) -> Option<&Ipset> { + if let Some(guest_config) = self.guests().get(&vmid) { + return guest_config.ipset(name); + } + + log::warn!("trying to get ipset {name} for non-existing guest: #{vmid}"); + None + } + + pub fn ipset(&self, name: &RuleIpsetName, vmid: Option<Vmid>) -> Option<&Ipset> { + log::trace!("getting ipset {name:?}"); + + match name { + RuleIpsetName::Scoped(ipset_name) => match ipset_name.scope() { + IpsetScope::Sdn => self.sdn()?.ipset(ipset_name.name()), + IpsetScope::Datacenter => self.cluster().ipset(ipset_name.name()), + IpsetScope::Guest => { + vmid.and_then(|vmid| self.guest_ipset(ipset_name.name(), vmid)) + } + }, + RuleIpsetName::Legacy(legacy_ipset_name) => vmid + .and_then(|vmid| self.guest_ipset(legacy_ipset_name.as_ref(), vmid)) + .or_else(|| self.cluster().ipset(legacy_ipset_name.as_ref())), } } } diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs index ddf839b..30bc642 100644 --- a/proxmox-firewall/src/firewall.rs +++ b/proxmox-firewall/src/firewall.rs @@ -196,7 +196,7 @@ impl Firewall { let management_ips = HostConfig::management_ips()?; - let mut ipset = Ipset::from_parts(IpsetScope::Datacenter, "management"); + let mut ipset = Ipset::new(IpsetName::new(IpsetScope::Datacenter, "management")); ipset.reserve(management_ips.len()); let entries = management_ips.into_iter().map(IpsetEntry::from); @@ -229,13 +229,10 @@ impl Firewall { let guest_table = Self::guest_table(); if let Some(sdn_config) = self.config.sdn() { - let ipsets = sdn_config - .ipsets(None) - .map(|ipset| (ipset.name().to_string(), ipset)) - .collect(); + let ipsets = sdn_config.ipsets(); - self.create_ipsets(&mut commands, &ipsets, &cluster_host_table, None)?; - self.create_ipsets(&mut commands, &ipsets, &guest_table, None)?; + self.create_ipsets(&mut commands, ipsets, &cluster_host_table, None)?; + self.create_ipsets(&mut commands, ipsets, &guest_table, None)?; } if let Some(ipam_config) = self.config.ipam() { diff --git a/proxmox-firewall/src/object.rs b/proxmox-firewall/src/object.rs index 42423b9..5c18708 100644 --- a/proxmox-firewall/src/object.rs +++ b/proxmox-firewall/src/object.rs @@ -13,7 +13,7 @@ use proxmox_nftables::{ use proxmox_ve_config::{ firewall::{ ct_helper::CtHelperMacro, - types::{alias::AliasName, ipset::IpsetAddress, Alias, Ipset}, + types::{alias::RuleAliasName, ipset::IpsetAddress, Alias, Ipset}, }, guest::types::Vmid, }; @@ -29,7 +29,7 @@ pub(crate) struct NftObjectEnv<'a, 'b> { } impl NftObjectEnv<'_, '_> { - pub(crate) fn alias(&self, name: &AliasName) -> Option<&Alias> { + pub(crate) fn alias(&self, name: &RuleAliasName) -> Option<&Alias> { self.firewall_config.alias(name, self.vmid) } } diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs index 23e87a5..23bdfb4 100644 --- a/proxmox-firewall/src/rule.rs +++ b/proxmox-firewall/src/rule.rs @@ -14,14 +14,14 @@ use proxmox_ve_config::{ ct_helper::CtHelperMacro, fw_macros::{get_macro, FwMacro}, types::{ - alias::AliasName, - ipset::{Ipfilter, IpsetName}, + alias::RuleAliasName, + ipset::{Ipfilter, IpsetName, RuleIpsetName}, log::LogRateLimit, rule::{Direction, Kind, RuleGroup, Verdict as ConfigVerdict}, rule_match::{ Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp, }, - Alias, Rule, + Alias, Ipset, Rule, }, }, guest::types::Vmid, @@ -121,7 +121,11 @@ pub(crate) struct NftRuleEnv<'a> { } impl NftRuleEnv<'_> { - fn alias(&self, name: &AliasName) -> Option<&Alias> { + fn ipset(&self, name: &RuleIpsetName) -> Option<&Ipset> { + self.firewall_config.ipset(name, self.vmid) + } + + fn alias(&self, name: &RuleAliasName) -> Option<&Alias> { self.firewall_config.alias(name, self.vmid) } @@ -289,10 +293,14 @@ impl ToNftRules for RuleMatch { /// `name`. It matches the IPs contained in the ipset with the field given in `field_name`. fn handle_set( rules: &mut Vec<NftRule>, - name: &IpsetName, + name: &RuleIpsetName, field_name: &str, env: &NftRuleEnv, ) -> Result<(), Error> { + let Some(ipset) = env.ipset(name) else { + bail!("could not find ipset {name}"); + }; + let mut new_rules = rules .drain(..) .flat_map(|rule| { @@ -310,7 +318,7 @@ fn handle_set( field.clone(), Expression::set_name(&SetName::ipset_name( Family::V4, - name, + ipset.name(), env.vmid, false, )), @@ -321,7 +329,7 @@ fn handle_set( field, Expression::set_name(&SetName::ipset_name( Family::V4, - name, + ipset.name(), env.vmid, true, )), @@ -344,7 +352,7 @@ fn handle_set( field.clone(), Expression::set_name(&SetName::ipset_name( Family::V6, - name, + ipset.name(), env.vmid, false, )), @@ -355,7 +363,7 @@ fn handle_set( field, Expression::set_name(&SetName::ipset_name( Family::V6, - name, + ipset.name(), env.vmid, true, )), diff --git a/proxmox-firewall/tests/input/cluster.fw b/proxmox-firewall/tests/input/cluster.fw index 3be7a72..376d2f1 100644 --- a/proxmox-firewall/tests/input/cluster.fw +++ b/proxmox-firewall/tests/input/cluster.fw @@ -20,8 +20,10 @@ dc/network1 GROUP network1 -i eth0 IN ACCEPT -log nolog +IN ACCEPT -dest +network1 [group network1] IN ACCEPT -source dc/network1 -dest dc/network1 -log nolog +IN ACCEPT -source network1 -dest network1 -log nolog diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap index 71285af..1a19ea7 100644 --- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap +++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap @@ -1820,6 +1820,54 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" } } }, + { + "add": { + "rule": { + "family": "inet", + "table": "proxmox-firewall", + "chain": "group-network1-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "saddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "accept": null + } + ] + } + } + }, { "add": { "chain": { @@ -1897,6 +1945,82 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" } } }, + { + "add": { + "rule": { + "family": "inet", + "table": "proxmox-firewall", + "chain": "cluster-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": "@v4-dc/network1" + } + }, + { + "match": { + "op": "!=", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": "@v4-dc/network1-nomatch" + } + }, + { + "accept": null + } + ] + } + } + }, + { + "add": { + "rule": { + "family": "inet", + "table": "proxmox-firewall", + "chain": "cluster-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip6", + "field": "daddr" + } + }, + "right": "@v6-dc/network1" + } + }, + { + "match": { + "op": "!=", + "left": { + "payload": { + "protocol": "ip6", + "field": "daddr" + } + }, + "right": "@v6-dc/network1-nomatch" + } + }, + { + "accept": null + } + ] + } + } + }, { "add": { "rule": { @@ -4003,6 +4127,54 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")" } } }, + { + "add": { + "rule": { + "family": "bridge", + "table": "proxmox-firewall-guests", + "chain": "group-network1-in", + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "saddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "ip", + "field": "daddr" + } + }, + "right": { + "prefix": { + "addr": "172.16.100.0", + "len": 24 + } + } + } + }, + { + "accept": null + } + ] + } + } + }, { "add": { "chain": { -- 2.47.3 _______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
