Re: [pve-devel] [PATCH proxmox-firewall 13/37] config: firewall: add host specific config + option types

["Max Carrara" <m.carrara@proxmox.com>]

On Tue Apr 2, 2024 at 7:16 PM CEST, Stefan Hanreich wrote:
> Co-authored-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  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 0000000..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<bool>,
> +
> +    #[serde(default, with = "parse::serde_option_bool")]
> +    nftables: Option<bool>,
> +
> +    log_level_in: Option<LogLevel>,
> +    log_level_out: Option<LogLevel>,
> +
> +    #[serde(default, with = "parse::serde_option_bool")]
> +    log_nf_conntrack: Option<bool>,
> +    #[serde(default, with = "parse::serde_option_bool")]
> +    ndp: Option<bool>,
> +
> +    #[serde(default, with = "parse::serde_option_bool")]
> +    nf_conntrack_allow_invalid: Option<bool>,
> +
> +    #[serde(default, with = "parse::serde_option_conntrack_helpers")]
> +    nf_conntrack_helpers: Option<Vec<String>>,
> +
> +    #[serde(default, with = "parse::serde_option_number")]
> +    nf_conntrack_max: Option<i64>,
> +    #[serde(default, with = "parse::serde_option_number")]
> +    nf_conntrack_tcp_timeout_established: Option<i64>,
> +    #[serde(default, with = "parse::serde_option_number")]
> +    nf_conntrack_tcp_timeout_syn_recv: Option<i64>,
> +
> +    #[serde(default, with = "parse::serde_option_bool")]
> +    nosmurfs: Option<bool>,
> +
> +    #[serde(default, with = "parse::serde_option_bool")]
> +    protection_synflood: Option<bool>,
> +    #[serde(default, with = "parse::serde_option_number")]
> +    protection_synflood_burst: Option<i64>,
> +    #[serde(default, with = "parse::serde_option_number")]
> +    protection_synflood_rate: Option<i64>,
> +
> +    smurf_log_level: Option<LogLevel>,
> +    tcp_flags_log_level: Option<LogLevel>,
> +
> +    #[serde(default, with = "parse::serde_option_bool")]
> +    tcpflags: Option<bool>,
> +}
> +
> +#[derive(Debug, Default)]
> +pub struct Config {
> +    pub(crate) config: super::common::Config<Options>,
> +}
> +
> +impl Config {
> +    pub fn new() -> Self {
> +        Self {
> +            config: Default::default(),
> +        }
> +    }
> +
> +    pub fn parse<R: io::BufRead>(input: R) -> Result<Self, Error> {
> +        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<Vec<Cidr>, 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.config.options.nosmurfs.unwrap_or(true)
> +    }
> +
> +    pub fn block_smurfs_log_level(&self) -> LogLevel {
> +        self.config.options.smurf_log_level.unwrap_or_default()
> +    }
> +
> +    pub fn block_synflood(&self) -> bool {
> +        self.config.options.protection_synflood.unwrap_or(false)
> +    }
> +
> +    pub fn synflood_rate(&self) -> i64 {
> +        self.config.options.protection_synflood_rate.unwrap_or(200)
> +    }

Should maybe document such defaults in the docstring of the `pub`
function above?

> +
> +    pub fn synflood_burst(&self) -> i64 {
> +        self.config
> +            .options
> +            .protection_synflood_burst
> +            .unwrap_or(1000)
> +    }

Same here.

Also, numeric defaults like those could maaaaaybe be declared as a
`const` upfront (and documented). Technically, doing this for the
boolean defaults here in this patch wouldn't hurt either - I realize
that it's clear from the context of the code what's meant, but in this
case it would be solely for documentation purposes.

E.g. if the question "Does the firewall enable NDP by default?" arises,
one could just check the (docstrings of the) constants declared at the
top of the file, or even better, browse the docs generated by cargo if
they're not a developer.

This might seem a little pedantic, but e.g. altering or removing default
values can lead to a new major version in semver, so IMO it's best if
they're defined more explicitly somewhere.

> +
> +    pub fn block_invalid_tcp(&self) -> bool {
> +        self.config.options.tcpflags.unwrap_or(false)
> +    }
> +
> +    pub fn block_invalid_tcp_log_level(&self) -> LogLevel {
> +        self.config.options.tcp_flags_log_level.unwrap_or_default()
> +    }
> +
> +    pub fn block_invalid_conntrack(&self) -> bool {
> +        !self
> +            .config
> +            .options
> +            .nf_conntrack_allow_invalid
> +            .unwrap_or(false)
> +    }
> +
> +    pub fn nf_conntrack_max(&self) -> Option<i64> {
> +        self.config.options.nf_conntrack_max
> +    }
> +
> +    pub fn nf_conntrack_tcp_timeout_established(&self) -> Option<i64> {
> +        self.config.options.nf_conntrack_tcp_timeout_established
> +    }
> +
> +    pub fn nf_conntrack_tcp_timeout_syn_recv(&self) -> Option<i64> {
> +        self.config.options.nf_conntrack_tcp_timeout_syn_recv
> +    }
> +
> +    pub fn log_nf_conntrack(&self) -> bool {
> +        self.config.options.log_nf_conntrack.unwrap_or(false)
> +    }
> +
> +    pub fn conntrack_helpers(&self) -> Option<&Vec<String>> {
> +        self.config.options.nf_conntrack_helpers.as_ref()
> +    }
> +
> +    pub fn log_level(&self, dir: Direction) -> LogLevel {
> +        match dir {
> +            Direction::In => self.config.options.log_level_in.unwrap_or_default(),
> +            Direction::Out => self.config.options.log_level_out.unwrap_or_default(),
> +        }
> +    }
> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    use crate::firewall::types::{
> +        log::LogLevel,
> +        rule::{Kind, RuleGroup, Verdict},
> +        rule_match::{Ports, Protocol, RuleMatch, Udp},
> +    };
> +
> +    use super::*;
> +
> +    #[test]
> +    fn test_parse_config() {
> +        const CONFIG: &str = r#"
> +[OPTIONS]
> +enable: 1
> +nftables: 1
> +log_level_in: debug
> +log_level_out: emerg
> +log_nf_conntrack: 0
> +ndp: 1
> +nf_conntrack_allow_invalid: yes
> +nf_conntrack_helpers: ftp
> +nf_conntrack_max: 44000
> +nf_conntrack_tcp_timeout_established: 500000
> +nf_conntrack_tcp_timeout_syn_recv: 44
> +nosmurfs: no
> +protection_synflood: 1
> +protection_synflood_burst: 2500
> +protection_synflood_rate: 300
> +smurf_log_level: notice
> +tcp_flags_log_level: nolog
> +tcpflags: yes
> +
> +[RULES]
> +
> +GROUP tgr -i eth0 # acomm
> +IN ACCEPT -p udp -dport 33 -sport 22 -log warning
> +
> +"#;
> +
> +        let mut config = CONFIG.as_bytes();
> +        let config = Config::parse(&mut config).unwrap();
> +
> +        assert_eq!(
> +            config.config.options,
> +            Options {
> +                enable: Some(true),
> +                nftables: Some(true),
> +                log_level_in: Some(LogLevel::Debug),
> +                log_level_out: Some(LogLevel::Emergency),
> +                log_nf_conntrack: Some(false),
> +                ndp: Some(true),
> +                nf_conntrack_allow_invalid: Some(true),
> +                nf_conntrack_helpers: Some(vec!["ftp".to_string()]),
> +                nf_conntrack_max: Some(44000),
> +                nf_conntrack_tcp_timeout_established: Some(500000),
> +                nf_conntrack_tcp_timeout_syn_recv: Some(44),
> +                nosmurfs: Some(false),
> +                protection_synflood: Some(true),
> +                protection_synflood_burst: Some(2500),
> +                protection_synflood_rate: Some(300),
> +                smurf_log_level: Some(LogLevel::Notice),
> +                tcp_flags_log_level: Some(LogLevel::Nolog),
> +                tcpflags: Some(true),
> +            }
> +        );
> +
> +        assert_eq!(config.config.rules.len(), 2);
> +
> +        assert_eq!(
> +            config.config.rules[0],
> +            Rule {
> +                disabled: false,
> +                comment: Some("acomm".to_string()),
> +                kind: Kind::Group(RuleGroup {
> +                    group: "tgr".to_string(),
> +                    iface: Some("eth0".to_string()),
> +                }),
> +            },
> +        );
> +
> +        assert_eq!(
> +            config.config.rules[1],
> +            Rule {
> +                disabled: false,
> +                comment: None,
> +                kind: Kind::Match(RuleMatch {
> +                    dir: Direction::In,
> +                    verdict: Verdict::Accept,
> +                    proto: Some(Protocol::Udp(Udp::new(Ports::from_u16(22, 33)))),
> +                    log: Some(LogLevel::Warning),
> +                    ..Default::default()
> +                }),
> +            },
> +        );
> +
> +        Config::parse("[ALIASES]\ntest 127.0.0.1".as_bytes())
> +            .expect_err("host config cannot contain aliases");
> +
> +        Config::parse("[GROUP test]".as_bytes()).expect_err("host config cannot contain groups");
> +
> +        Config::parse("[IPSET test]".as_bytes()).expect_err("host config cannot contain ipsets");
> +    }
> +}
> diff --git a/proxmox-ve-config/src/firewall/mod.rs b/proxmox-ve-config/src/firewall/mod.rs
> index 82689c3..85fe6c4 100644
> --- a/proxmox-ve-config/src/firewall/mod.rs
> +++ b/proxmox-ve-config/src/firewall/mod.rs
> @@ -1,5 +1,6 @@
>  pub mod cluster;
>  pub mod common;
> +pub mod host;
>  pub mod ports;
>  pub mod types;
>