fix(config): tighten CIDR, interface and list validation [P1]
parse_cidr_v4 split on '/' and read the first two components without checking for a third, so "10.8.1.0/24/garbage" parsed as a valid /24 with the tail silently dropped. The value reaches nftables rules and tc filters, so accepting malformed input meant applying rules derived from a config the operator did not write. Now a second separator is an error, and the negative cases are pinned by a test: missing prefix, empty prefix, double separator, negative and out-of-range prefixes, a malformed octet, an empty string, and a missing address. Interface names were only checked for being non-empty, and only for ifb_name. They are passed straight to ip and tc, so a name that cannot exist produces a confusing runtime failure instead of a config error. Validation now covers amnezia.interface_candidates and traffic.ifb_name: non-empty, within the Linux IFNAMSIZ-1 limit of 15 characters, not '.' or '..', and free of whitespace, control characters, '/' and ':'. interface_patterns get the same rules plus one specific to the matcher: only a single trailing '*' is supported, so a pattern like "*wg" or "a*g*" would have silently never matched anything. A bare "*" is rejected outright - matching every interface is not a fallback, it is a way to attach shaping to the wrong device. container_names are matched exactly against docker ps output, so a name outside Docker's own [a-zA-Z0-9][a-zA-Z0-9_.-]* grammar can never be found. That is now a config error rather than a container that mysteriously never appears. All four ordered lists - container_names, interface_candidates, interface_patterns, wg_binaries - are checked for blank and duplicate entries through one shared helper. A blank entry becomes an unmatchable candidate; a duplicate makes an ordered list lie about its priority. Neither is normalized away silently. Seven tests added, covering accepted and rejected values for each rule. cargo test --workspace: 155 + 4 passed. clippy -D warnings clean.
This commit is contained in:
+221
-15
@@ -167,23 +167,30 @@ impl Config {
|
||||
}
|
||||
|
||||
pub fn validate_config(&self) -> anyhow::Result<()> {
|
||||
if self.amnezia.container_names.is_empty() {
|
||||
anyhow::bail!("amnezia.container_names must not be empty");
|
||||
check_non_empty_unique(&self.amnezia.container_names, "amnezia.container_names")?;
|
||||
for name in &self.amnezia.container_names {
|
||||
validate_container_name(name)?;
|
||||
}
|
||||
if self.amnezia.interface_candidates.is_empty() {
|
||||
anyhow::bail!("amnezia.interface_candidates must not be empty");
|
||||
|
||||
check_non_empty_unique(
|
||||
&self.amnezia.interface_candidates,
|
||||
"amnezia.interface_candidates",
|
||||
)?;
|
||||
for name in &self.amnezia.interface_candidates {
|
||||
validate_interface_name(name, "amnezia.interface_candidates")?;
|
||||
}
|
||||
if self.amnezia.interface_patterns.is_empty() {
|
||||
anyhow::bail!("amnezia.interface_patterns must not be empty");
|
||||
}
|
||||
if self.amnezia.wg_binaries.is_empty() {
|
||||
anyhow::bail!("amnezia.wg_binaries must not be empty");
|
||||
|
||||
check_non_empty_unique(
|
||||
&self.amnezia.interface_patterns,
|
||||
"amnezia.interface_patterns",
|
||||
)?;
|
||||
for pattern in &self.amnezia.interface_patterns {
|
||||
validate_interface_pattern(pattern)?;
|
||||
}
|
||||
|
||||
check_non_empty_unique(&self.amnezia.wg_binaries, "amnezia.wg_binaries")?;
|
||||
for binary in &self.amnezia.wg_binaries {
|
||||
// Values are executed inside the container: keep them bare command names.
|
||||
if binary.is_empty() {
|
||||
anyhow::bail!("amnezia.wg_binaries must not contain an empty value");
|
||||
}
|
||||
if binary.chars().any(char::is_whitespace) {
|
||||
anyhow::bail!("amnezia.wg_binaries must not contain whitespace: {binary}");
|
||||
}
|
||||
@@ -196,9 +203,7 @@ impl Config {
|
||||
if self.traffic.class_start_id < 10 {
|
||||
anyhow::bail!("traffic.class_start_id must be >= 10");
|
||||
}
|
||||
if self.traffic.ifb_name.trim().is_empty() {
|
||||
anyhow::bail!("traffic.ifb_name must not be empty");
|
||||
}
|
||||
validate_interface_name(&self.traffic.ifb_name, "traffic.ifb_name")?;
|
||||
if self.traffic.peer_limit.trim().is_empty() {
|
||||
anyhow::bail!("traffic.peer_limit must not be empty");
|
||||
}
|
||||
@@ -505,6 +510,87 @@ fn tool_exists(tool: &str) -> bool {
|
||||
false
|
||||
}
|
||||
|
||||
/// Linux caps interface names at `IFNAMSIZ - 1`.
|
||||
const MAX_INTERFACE_NAME_LEN: usize = 15;
|
||||
|
||||
/// Rejects list entries that are blank or repeated.
|
||||
///
|
||||
/// A blank entry silently becomes an unmatchable candidate, and a duplicate makes the
|
||||
/// ordered lists lie about their priority, so both are configuration errors rather than
|
||||
/// something to normalize away.
|
||||
fn check_non_empty_unique(values: &[String], field: &str) -> anyhow::Result<()> {
|
||||
if values.is_empty() {
|
||||
anyhow::bail!("{field} must not be empty");
|
||||
}
|
||||
let mut seen = std::collections::BTreeSet::new();
|
||||
for value in values {
|
||||
if value.trim().is_empty() {
|
||||
anyhow::bail!("{field} must not contain an empty value");
|
||||
}
|
||||
if !seen.insert(value.as_str()) {
|
||||
anyhow::bail!("{field} contains a duplicate value: {value}");
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Validates a name that will be passed to `ip`/`tc` as an interface.
|
||||
fn validate_interface_name(name: &str, field: &str) -> anyhow::Result<()> {
|
||||
if name.is_empty() {
|
||||
anyhow::bail!("{field} must not be empty");
|
||||
}
|
||||
if name.len() > MAX_INTERFACE_NAME_LEN {
|
||||
anyhow::bail!(
|
||||
"{field} exceeds the Linux interface name limit of {MAX_INTERFACE_NAME_LEN} characters: {name}"
|
||||
);
|
||||
}
|
||||
if name == "." || name == ".." {
|
||||
anyhow::bail!("{field} must not be '.' or '..': {name}");
|
||||
}
|
||||
if let Some(bad) = name
|
||||
.chars()
|
||||
.find(|c| c.is_whitespace() || c.is_control() || *c == '/' || *c == ':')
|
||||
{
|
||||
anyhow::bail!("{field} contains an invalid character {bad:?}: {name}");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Same rules as an interface name, but a single trailing `*` is allowed.
|
||||
///
|
||||
/// Only the suffix form is supported by the matcher, so a pattern with `*` anywhere else
|
||||
/// would silently never match.
|
||||
fn validate_interface_pattern(pattern: &str) -> anyhow::Result<()> {
|
||||
let field = "amnezia.interface_patterns";
|
||||
let stem = pattern.strip_suffix('*').unwrap_or(pattern);
|
||||
if stem.contains('*') {
|
||||
anyhow::bail!("{field} supports only a single trailing '*': {pattern}");
|
||||
}
|
||||
if stem.is_empty() {
|
||||
anyhow::bail!("{field} must not match every interface: {pattern}");
|
||||
}
|
||||
validate_interface_name(stem, field)
|
||||
}
|
||||
|
||||
/// Docker container names are `[a-zA-Z0-9][a-zA-Z0-9_.-]*`; matching is exact, so a name
|
||||
/// that cannot exist would silently never be found.
|
||||
fn validate_container_name(name: &str) -> anyhow::Result<()> {
|
||||
let field = "amnezia.container_names";
|
||||
let mut chars = name.chars();
|
||||
let first = chars
|
||||
.next()
|
||||
.ok_or_else(|| anyhow::anyhow!("{field} must not contain an empty value"))?;
|
||||
if !first.is_ascii_alphanumeric() {
|
||||
anyhow::bail!("{field} must start with an alphanumeric character: {name}");
|
||||
}
|
||||
if let Some(bad) =
|
||||
chars.find(|c| !(c.is_ascii_alphanumeric() || *c == '_' || *c == '.' || *c == '-'))
|
||||
{
|
||||
anyhow::bail!("{field} contains an invalid character {bad:?}: {name}");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub fn parse_cidr_v4(cidr: &str) -> anyhow::Result<(Ipv4Addr, u8)> {
|
||||
let mut parts = cidr.split('/');
|
||||
let ip = parts
|
||||
@@ -515,6 +601,11 @@ pub fn parse_cidr_v4(cidr: &str) -> anyhow::Result<(Ipv4Addr, u8)> {
|
||||
.next()
|
||||
.ok_or_else(|| anyhow::anyhow!("invalid cidr: {cidr}"))?
|
||||
.parse::<u8>()?;
|
||||
// Without this, "10.8.1.0/24/garbage" parsed as a valid /24 and the trailing junk was
|
||||
// silently dropped.
|
||||
if parts.next().is_some() {
|
||||
anyhow::bail!("invalid cidr: {cidr} has more than one '/' separator");
|
||||
}
|
||||
if prefix > 32 {
|
||||
anyhow::bail!("invalid prefix in cidr: {cidr}");
|
||||
}
|
||||
@@ -584,6 +675,121 @@ mod tests {
|
||||
assert!(err.is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_cidr_accepts_well_formed_values() {
|
||||
assert_eq!(
|
||||
parse_cidr_v4("10.8.1.0/24").expect("cidr"),
|
||||
("10.8.1.0".parse().expect("ip"), 24)
|
||||
);
|
||||
assert_eq!(
|
||||
parse_cidr_v4("0.0.0.0/0").expect("cidr"),
|
||||
("0.0.0.0".parse().expect("ip"), 0)
|
||||
);
|
||||
}
|
||||
|
||||
/// "10.8.1.0/24/garbage" used to parse as a valid /24 with the tail silently dropped.
|
||||
#[test]
|
||||
fn parse_cidr_rejects_malformed_values() {
|
||||
for bad in [
|
||||
"10.8.1.0",
|
||||
"10.8.1.0/",
|
||||
"10.8.1.0/24/extra",
|
||||
"10.8.1.0//24",
|
||||
"10.8.1.0/-1",
|
||||
"10.8.1.0/33",
|
||||
"10.8.1.999/24",
|
||||
"",
|
||||
"/24",
|
||||
] {
|
||||
assert!(parse_cidr_v4(bad).is_err(), "must reject cidr: {bad:?}");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn interface_name_rules() {
|
||||
for good in ["awg0", "wg0", "ifb0", "amnezia0", "abcdefghijklmno"] {
|
||||
assert!(
|
||||
validate_interface_name(good, "f").is_ok(),
|
||||
"must accept: {good}"
|
||||
);
|
||||
}
|
||||
for bad in [
|
||||
"",
|
||||
"abcdefghijklmnop",
|
||||
"eth 0",
|
||||
"eth/0",
|
||||
"eth:0",
|
||||
"eth\u{0}0",
|
||||
".",
|
||||
"..",
|
||||
] {
|
||||
assert!(
|
||||
validate_interface_name(bad, "f").is_err(),
|
||||
"must reject: {bad:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn interface_pattern_rules() {
|
||||
for good in ["awg*", "wg*", "tun*", "awg0"] {
|
||||
assert!(
|
||||
validate_interface_pattern(good).is_ok(),
|
||||
"must accept: {good}"
|
||||
);
|
||||
}
|
||||
// A bare '*' would match every interface, and '*' is only supported as a suffix.
|
||||
for bad in ["*", "a*g*", "*wg", "aw g*"] {
|
||||
assert!(
|
||||
validate_interface_pattern(bad).is_err(),
|
||||
"must reject: {bad:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn container_name_rules() {
|
||||
for good in ["amnezia-awg2", "amnezia_awg", "awg.1", "a"] {
|
||||
assert!(validate_container_name(good).is_ok(), "must accept: {good}");
|
||||
}
|
||||
for bad in ["", "-awg", ".awg", "amnezia awg", "amnezia/awg"] {
|
||||
assert!(
|
||||
validate_container_name(bad).is_err(),
|
||||
"must reject: {bad:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lists_reject_blank_and_duplicate_entries() {
|
||||
let mut cfg = base_config();
|
||||
cfg.amnezia.container_names = vec!["a".into(), "".into()];
|
||||
assert!(cfg.validate_config().is_err());
|
||||
|
||||
let mut cfg = base_config();
|
||||
cfg.amnezia.container_names = vec!["a".into(), "a".into()];
|
||||
assert!(cfg.validate_config().is_err());
|
||||
|
||||
let mut cfg = base_config();
|
||||
cfg.amnezia.interface_candidates = vec!["awg0".into(), "awg0".into()];
|
||||
assert!(cfg.validate_config().is_err());
|
||||
|
||||
let mut cfg = base_config();
|
||||
cfg.amnezia.wg_binaries = vec!["awg".into(), "awg".into()];
|
||||
assert!(cfg.validate_config().is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ifb_name_is_validated_as_an_interface() {
|
||||
let mut cfg = base_config();
|
||||
cfg.traffic.ifb_name = "ifb 0".into();
|
||||
assert!(cfg.validate_config().is_err());
|
||||
|
||||
let mut cfg = base_config();
|
||||
cfg.traffic.ifb_name = "abcdefghijklmnop".into();
|
||||
assert!(cfg.validate_config().is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ip_membership() {
|
||||
let ok = ip_in_cidr("10.8.1.10".parse().expect("ip"), "10.8.1.0/24").expect("cidr");
|
||||
|
||||
Reference in New Issue
Block a user