From 7bc22886dbb8a043284b2d2a4084f7e564e3da66 Mon Sep 17 00:00:00 2001 From: Zvonko Kaiser Date: Tue, 4 Jun 2024 18:24:18 +0000 Subject: [PATCH] implemented correct k8s validation Signed-off-by: Zvonko Kaiser --- Cargo.toml | 8 +- src/annotations.rs | 10 +- src/device.rs | 6 +- src/internal/validation/k8s/objectmeta.rs | 106 ++++++++++- src/internal/validation/k8s/validation.rs | 207 ++++++++++++++++++++++ src/internal/validation/validate.rs | 7 - src/lib.rs | 1 - src/validations.rs | 111 ------------ 8 files changed, 322 insertions(+), 134 deletions(-) delete mode 100644 src/validations.rs diff --git a/Cargo.toml b/Cargo.toml index 71ae240..9c24b71 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ path = "src/bin/validate.rs" # Path to the source file [dependencies] oci-spec = { git = "https://github.com/containers/oci-spec-rs.git", rev = "d338cf8", features = ["runtime"] } anyhow = "1.0" -clap = "4.5.3" +clap = "4.5.4" serde_json = "1.0" jsonschema = "0.18.0" once_cell = "1.8.0" @@ -36,11 +36,13 @@ notify = "6.1.1" serde = "1.0.131" serde_derive = "1.0.131" libc = "0.2.112" -nix = "0.29.0" +nix = "0.24.2" lazy_static = "1.2" path-clean = "1.0.1" serde_yaml = "0.9.33" -semver = "1.0.23" +semver = "1.0.22" +regex = "1.5.4" +const_format = "0.2.32" [dev-dependencies] tempfile = "3.1.0" diff --git a/src/annotations.rs b/src/annotations.rs index 4ce6f27..e6977f8 100644 --- a/src/annotations.rs +++ b/src/annotations.rs @@ -41,8 +41,8 @@ pub(crate) fn update_annotations( // are returned along with a non-nil error. The annotations are expected // to be formatted by, or in a compatible fashion to UpdateAnnotations(). #[allow(dead_code)] -pub(crate) fn parse_annotations( - annotations: HashMap, +pub fn parse_annotations( + annotations: &HashMap, ) -> Result<(Vec, Vec)> { let mut keys: Vec = Vec::new(); let mut devices: Vec = Vec::new(); @@ -58,7 +58,7 @@ pub(crate) fn parse_annotations( } devices.push(device.to_string()); } - keys.push(k); + keys.push(k.to_string()); } Ok((keys, devices)) @@ -169,8 +169,8 @@ mod tests { .to_string(), ); - assert!(parse_annotations(cdi_devices.clone()).is_ok()); - let (keys, devices) = parse_annotations(cdi_devices).unwrap(); + assert!(parse_annotations(&cdi_devices).is_ok()); + let (keys, devices) = parse_annotations(&cdi_devices).unwrap(); assert_eq!(keys.len(), 3); assert_eq!(devices.len(), 3); } diff --git a/src/device.rs b/src/device.rs index 6be02a2..09e84b1 100644 --- a/src/device.rs +++ b/src/device.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashMap}; +use std::collections::BTreeMap; use anyhow::{anyhow, Context, Result}; use oci_spec::runtime as oci; @@ -8,7 +8,7 @@ use crate::{ parser::{qualified_name, validate_device_name}, spec::Spec, specs::config::Device as CDIDevice, - validations::validate_spec_annotations, + internal::validation::validate::validate_spec_annotations, }; // Device represents a CDI device of a Spec. @@ -82,7 +82,7 @@ impl Device { validate_device_name(&self.cdi_device.name).context("validate device name failed")?; let name = self.get_qualified_name(); - let annotations: &HashMap = + let annotations: &BTreeMap = & as Clone>::clone(&self.cdi_device.annotations) .into_iter() .collect(); diff --git a/src/internal/validation/k8s/objectmeta.rs b/src/internal/validation/k8s/objectmeta.rs index 4da1bb5..9a06c9b 100644 --- a/src/internal/validation/k8s/objectmeta.rs +++ b/src/internal/validation/k8s/objectmeta.rs @@ -1,7 +1,105 @@ -use anyhow::Result; +use anyhow::{Error, Result}; use std::collections::BTreeMap; -pub fn validate_annotations(_annotations: &BTreeMap, _path: &str) -> Result<()> { - // Implement the actual validation logic or import the function from your k8s module - Ok(()) +const TOTAL_ANNOTATION_SIZE_LIMIT: usize = 256 * 1024; // 256 kB + +use super::validation::is_qualified_name; + +pub fn validate_annotations(annotations: &BTreeMap, path: &str) -> Result<()> { + let mut errs = Vec::new(); + + for k in annotations.keys() { + for msg in is_qualified_name(&k.to_lowercase()) { + errs.push(format!("{}.{} is invalid: {}", path, k, msg)); + } + } + + if let Err(err) = validate_annotations_size(annotations) { + errs.push(format!("{} is too long: {}", path, err)); + } + + if errs.is_empty() { + Ok(()) + } else { + Err(Error::msg(errs.join(", "))) + } +} + +fn validate_annotations_size(annotations: &BTreeMap) -> Result<()> { + let total_size: usize = annotations.iter().map(|(k, v)| k.len() + v.len()).sum(); + + if total_size > TOTAL_ANNOTATION_SIZE_LIMIT { + Err(Error::msg(format!( + "annotations size {} is larger than limit {}", + total_size, TOTAL_ANNOTATION_SIZE_LIMIT + ))) + } else { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::super::super::validate::validate_spec_annotations; + use super::validate_annotations; + + use std::collections::BTreeMap; + + #[test] + fn test_validate_annotations() { + let mut annotations = BTreeMap::new(); + annotations.insert( + "cdi.k8s.io/vfio17".to_string(), + "nvidia.com/gpu=0".to_string(), + ); + annotations.insert( + "cdi.k8s.io/vfio18".to_string(), + "nvidia.com/gpu=1".to_string(), + ); + annotations.insert( + "cdi.k8s.io/vfio19".to_string(), + "nvidia.com/gpu=all".to_string(), + ); + let path = "test.annotations"; + + assert!(validate_annotations(&annotations, path).is_ok()); + + let mut large_annotations = BTreeMap::new(); + let long_value = "CDI".repeat(super::TOTAL_ANNOTATION_SIZE_LIMIT + 1); + large_annotations.insert("CDIKEY".to_string(), long_value); + assert!(validate_annotations(&large_annotations, path).is_err()); + + let mut invalid_annotations = BTreeMap::new(); + invalid_annotations.insert("inv$$alid_CDIKEY".to_string(), "inv$$alid_CDIVAL".to_string()); + assert!(validate_annotations(&invalid_annotations, path).is_err()); + } + + #[test] + fn test_validate_spec_annotations() { + let mut annotations = BTreeMap::new(); + annotations.insert( + "cdi.k8s.io/vfio17".to_string(), + "nvidia.com/gpu=0".to_string(), + ); + annotations.insert( + "cdi.k8s.io/vfio18".to_string(), + "nvidia.com/gpu=1".to_string(), + ); + annotations.insert( + "cdi.k8s.io/vfio19".to_string(), + "nvidia.com/gpu=all".to_string(), + ); + + assert!(validate_spec_annotations("", &annotations).is_ok()); + assert!(validate_spec_annotations("CDITEST", &annotations).is_ok()); + + let mut large_annotations = BTreeMap::new(); + let long_value = "CDI".repeat(super::TOTAL_ANNOTATION_SIZE_LIMIT + 1); + large_annotations.insert("CDIKEY".to_string(), long_value); + assert!(validate_spec_annotations("", &large_annotations).is_err()); + + let mut invalid_annotations = BTreeMap::new(); + invalid_annotations.insert("inva$$lid_CDIKEY".to_string(), "inval$$id_CDIVAL".to_string()); + assert!(validate_spec_annotations("CDITEST", &invalid_annotations).is_err()); + } } diff --git a/src/internal/validation/k8s/validation.rs b/src/internal/validation/k8s/validation.rs index 8b13789..b07f2f2 100644 --- a/src/internal/validation/k8s/validation.rs +++ b/src/internal/validation/k8s/validation.rs @@ -1 +1,208 @@ +use lazy_static::lazy_static; +use regex::Regex; +use const_format::concatcp; +const QNAME_CHAR_FMT: &str = "[A-Za-z0-9]"; +const QNAME_EXT_CHAR_FMT: &str = "[-A-Za-z0-9_.]"; +const QUALIFIED_NAME_FMT: &str = concatcp!("(", QNAME_CHAR_FMT, QNAME_EXT_CHAR_FMT, "*)?", QNAME_CHAR_FMT); +const QUALIFIED_NAME_ERR_MSG: &str = "must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character"; +const QUALIFIED_NAME_MAX_LENGTH: usize = 63; + +const LABEL_VALUE_FMT: &str = concatcp!("(", QUALIFIED_NAME_FMT, ")?"); +const LABEL_VALUE_ERR_MSG: &str = "a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character"; +const LABEL_VALUE_MAX_LENGTH: usize = 63; + +const DNS1123_LABEL_FMT: &str = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"; +const DNS1123_LABEL_ERR_MSG: &str = "a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character"; +const DNS1123_LABEL_MAX_LENGTH: usize = 63; + +const DNS1123_SUBDOMAIN_FMT: &str = concatcp!(DNS1123_LABEL_FMT, "(\\.", DNS1123_LABEL_FMT, ")*"); +const DNS1123_SUBDOMAIN_ERR_MSG: &str = "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character"; +const DNS1123_SUBDOMAIN_MAX_LENGTH: usize = 253; + +const DNS1035_LABEL_FMT: &str = "[a-z]([-a-z0-9]*[a-z0-9])?"; +const DNS1035_LABEL_ERR_MSG: &str = "a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character"; +const DNS1035_LABEL_MAX_LENGTH: usize = 63; + +const WILDCARD_DNS1123_SUBDOMAIN_FMT: &str = concatcp!("\\*\\.", DNS1123_SUBDOMAIN_FMT); +const WILDCARD_DNS1123_SUBDOMAIN_ERR_MSG: &str = "a wildcard DNS-1123 subdomain must start with '*.', followed by a valid DNS subdomain, which must consist of lower case alphanumeric characters, '-' or '.' and end with an alphanumeric character"; + +lazy_static! { + static ref QUALIFIED_NAME_REGEXP: Regex = + Regex::new(&format!("^{}$", QUALIFIED_NAME_FMT)).unwrap(); + static ref LABEL_VALUE_REGEXP: Regex = Regex::new(&format!("^{}$", LABEL_VALUE_FMT)).unwrap(); + static ref DNS1123_LABEL_REGEXP: Regex = + Regex::new(&format!("^{}$", DNS1123_LABEL_FMT)).unwrap(); + static ref DNS1123_SUBDOMAIN_REGEXP: Regex = + Regex::new(&format!("^{}$", DNS1123_SUBDOMAIN_FMT)).unwrap(); + static ref DNS1035_LABEL_REGEXP: Regex = + Regex::new(&format!("^{}$", DNS1035_LABEL_FMT)).unwrap(); + static ref WILDCARD_DNS1123_SUBDOMAIN_REGEXP: Regex = + Regex::new(&format!("^{}$", WILDCARD_DNS1123_SUBDOMAIN_FMT)).unwrap(); +} + +pub fn is_qualified_name(value: &str) -> Vec { + let mut errs = Vec::new(); + let parts: Vec<&str> = value.split('/').collect(); + let name: &str; + + match parts.len() { + 1 => { + name = parts[0]; + } + 2 => { + let prefix = parts[0]; + name = parts[1]; + if prefix.is_empty() { + errs.push(format!("prefix part {}", empty_error())); + } else { + let msgs = is_dns1123_subdomain(prefix); + if msgs.is_empty() { + errs.extend(prefix_each(&msgs, "prefix part ")); + } + } + } + _ => { + return vec![format!( + "a qualified name {} with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')", + regex_error(QUALIFIED_NAME_ERR_MSG, QUALIFIED_NAME_FMT, &["MyName", "my.name", "123-abc"]) + )]; + } + } + + if name.is_empty() { + errs.push(format!("name part {}", empty_error())); + } else if name.len() > QUALIFIED_NAME_MAX_LENGTH { + errs.push(format!( + "name part {}", + max_len_error(QUALIFIED_NAME_MAX_LENGTH) + )); + } + + if !QUALIFIED_NAME_REGEXP.is_match(name) { + errs.push(format!( + "name part {}", + regex_error( + QUALIFIED_NAME_ERR_MSG, + QUALIFIED_NAME_FMT, + &["MyName", "my.name", "123-abc"] + ) + )); + } + + errs +} + +#[allow(dead_code)] +fn is_valid_label_value(value: &str) -> Vec { + let mut errs = Vec::new(); + if value.len() > LABEL_VALUE_MAX_LENGTH { + errs.push(max_len_error(LABEL_VALUE_MAX_LENGTH)); + } + if !LABEL_VALUE_REGEXP.is_match(value) { + errs.push(regex_error( + LABEL_VALUE_ERR_MSG, + LABEL_VALUE_FMT, + &["MyValue", "my_value", "12345"], + )); + } + errs +} + +#[allow(dead_code)] +fn is_dns1123_label(value: &str) -> Vec { + let mut errs = Vec::new(); + if value.len() > DNS1123_LABEL_MAX_LENGTH { + errs.push(max_len_error(DNS1123_LABEL_MAX_LENGTH)); + } + if !DNS1123_LABEL_REGEXP.is_match(value) { + errs.push(regex_error( + DNS1123_LABEL_ERR_MSG, + DNS1123_LABEL_FMT, + &["my-name", "123-abc"], + )); + } + errs +} + +fn is_dns1123_subdomain(value: &str) -> Vec { + let mut errs = Vec::new(); + if value.len() > DNS1123_SUBDOMAIN_MAX_LENGTH { + errs.push(max_len_error(DNS1123_SUBDOMAIN_MAX_LENGTH)); + } + if !DNS1123_SUBDOMAIN_REGEXP.is_match(value) { + errs.push(regex_error( + DNS1123_SUBDOMAIN_ERR_MSG, + DNS1123_SUBDOMAIN_FMT, + &["example.com"], + )); + } + errs +} + +#[allow(dead_code)] +fn is_dns1035_label(value: &str) -> Vec { + let mut errs = Vec::new(); + if value.len() > DNS1035_LABEL_MAX_LENGTH { + errs.push(max_len_error(DNS1035_LABEL_MAX_LENGTH)); + } + if !DNS1035_LABEL_REGEXP.is_match(value) { + errs.push(regex_error( + DNS1035_LABEL_ERR_MSG, + DNS1035_LABEL_FMT, + &["my-name", "abc-123"], + )); + } + errs +} + +#[allow(dead_code)] +fn is_wildcard_dns1123_subdomain(value: &str) -> Vec { + let mut errs = Vec::new(); + if value.len() > DNS1123_SUBDOMAIN_MAX_LENGTH { + errs.push(max_len_error(DNS1123_SUBDOMAIN_MAX_LENGTH)); + } + if !WILDCARD_DNS1123_SUBDOMAIN_REGEXP.is_match(value) { + errs.push(regex_error( + WILDCARD_DNS1123_SUBDOMAIN_ERR_MSG, + WILDCARD_DNS1123_SUBDOMAIN_FMT, + &["*.example.com"], + )); + } + errs +} + +fn max_len_error(max_length: usize) -> String { + format!("must be no more than {} characters", max_length) +} + +fn regex_error(err_msg: &str, regex_fmt: &str, examples: &[&str]) -> String { + if examples.is_empty() { + format!("{} (regex used for validation is '{}')", err_msg, regex_fmt) + } else { + let examples_str = examples + .iter() + .map(|&e| format!("'{}'", e)) + .collect::>() + .join(" or "); + format!( + "{} (e.g. {}. regex used for validation is '{}')", + err_msg, examples_str, regex_fmt + ) + } +} + +fn empty_error() -> &'static str { + "must be non-empty" +} + +fn prefix_each(msgs: &[String], prefix: &str) -> Vec { + msgs.iter() + .map(|msg| format!("{}{}", prefix, msg)) + .collect() +} + +#[allow(dead_code)] +fn inclusive_range_error(lo: usize, hi: usize) -> String { + format!("must be between {} and {}, inclusive", lo, hi) +} diff --git a/src/internal/validation/validate.rs b/src/internal/validation/validate.rs index 68824eb..823cde0 100644 --- a/src/internal/validation/validate.rs +++ b/src/internal/validation/validate.rs @@ -3,13 +3,6 @@ use anyhow::Result; use std::collections::BTreeMap; pub fn validate_spec_annotations(name: &str, annotations: &BTreeMap) -> Result<()> { - if annotations.is_empty() { - return Ok(()); - } - validate_annotations_inner(name, annotations) -} - -fn validate_annotations_inner(name: &str, annotations: &BTreeMap) -> Result<()> { let path = if !name.is_empty() { format!("{}.annotations", name) } else { diff --git a/src/lib.rs b/src/lib.rs index 4e98a25..c6b855c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,7 +12,6 @@ pub mod spec; pub mod spec_dirs; pub mod specs; pub mod utils; -pub mod validations; pub mod version; #[cfg(test)] diff --git a/src/validations.rs b/src/validations.rs deleted file mode 100644 index 3142f05..0000000 --- a/src/validations.rs +++ /dev/null @@ -1,111 +0,0 @@ -use std::collections::HashMap; - -use anyhow::Result; - -use crate::parser::parse_qualified_name; - -const TOTAL_ANNOTATION_SIZE_LIMIT: usize = 256 * 1024; // 256 kB - -#[allow(dead_code)] -pub fn validate_annotations(annotations: &HashMap) -> Result<(), Vec> { - let mut errors = Vec::new(); - let total_size: usize = annotations.iter().map(|(k, v)| k.len() + v.len()).sum(); - - if total_size > TOTAL_ANNOTATION_SIZE_LIMIT { - errors.push(format!( - "annotations size {} is larger than limit {}", - total_size, TOTAL_ANNOTATION_SIZE_LIMIT - )); - } - - for (key, value) in annotations { - if let Err(msg) = parse_qualified_name(value) { - errors.push(format!("{}:{} is invalid: {}", key, value, msg)); - } - } - - if errors.is_empty() { - Ok(()) - } else { - Err(errors) - } -} - -#[allow(dead_code)] -pub fn validate_spec_annotations( - name: &str, - annotations: &HashMap, -) -> Result<(), Vec> { - let path = if name.is_empty() { - "annotations".to_string() - } else { - format!("{}.annotations", name) - }; - - validate_annotations(annotations).map_err(|mut errors| { - errors.iter_mut().for_each(|error| { - error.insert_str(0, &format!("{}: ", path)); - }); - errors - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_validate_annotations() { - let mut annotations = HashMap::new(); - annotations.insert( - "cdi.k8s.io/vfio17".to_string(), - "nvidia.com/gpu=0".to_string(), - ); - annotations.insert( - "cdi.k8s.io/vfio18".to_string(), - "nvidia.com/gpu=1".to_string(), - ); - annotations.insert( - "cdi.k8s.io/vfio19".to_string(), - "nvidia.com/gpu=all".to_string(), - ); - assert!(validate_annotations(&annotations).is_ok()); - - let mut large_annotations = HashMap::new(); - let long_value = "CDI".repeat(TOTAL_ANNOTATION_SIZE_LIMIT + 1); - large_annotations.insert("CDIKEY".to_string(), long_value); - assert!(validate_annotations(&large_annotations).is_err()); - - let mut invalid_annotations = HashMap::new(); - invalid_annotations.insert("invalid_CDIKEY".to_string(), "invalied_CDIVAL".to_string()); - assert!(validate_annotations(&invalid_annotations).is_err()); - } - - #[test] - fn test_validate_spec_annotations() { - let mut annotations = HashMap::new(); - annotations.insert( - "cdi.k8s.io/vfio17".to_string(), - "nvidia.com/gpu=0".to_string(), - ); - annotations.insert( - "cdi.k8s.io/vfio18".to_string(), - "nvidia.com/gpu=1".to_string(), - ); - annotations.insert( - "cdi.k8s.io/vfio19".to_string(), - "nvidia.com/gpu=all".to_string(), - ); - assert!(validate_spec_annotations("", &annotations).is_ok()); - assert!(validate_spec_annotations("CDITEST", &annotations).is_ok()); - - let mut large_annotations = HashMap::new(); - let long_value = "CDI".repeat(TOTAL_ANNOTATION_SIZE_LIMIT + 1); - large_annotations.insert("CDIKEY".to_string(), long_value); - assert!(validate_spec_annotations("", &large_annotations).is_err()); - - let mut invalid_annotations = HashMap::new(); - invalid_annotations.insert("invalid_CDIKEY".to_string(), "invalied_CDIVAL".to_string()); - assert!(validate_spec_annotations("CDITEST", &invalid_annotations).is_err()); - } -}