From c1f31c0e4777bf0cfed0dd7f03249e9a61cd8cb9 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 19 Jun 2026 16:54:25 +0200 Subject: [PATCH] fix(backend): validate ansible vault_id entries before config generation (#9681) * fix(backend): validate ansible vault_id entries before config generation Co-Authored-By: Claude Opus 4.8 (1M context) * test(backend): cover ansible.cfg write-boundary vault_id validation Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../parsers/windmill-parser-yaml/src/lib.rs | 77 ++++++++++++++++++- .../windmill-worker/src/ansible_executor.rs | 35 ++++++++- 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/backend/parsers/windmill-parser-yaml/src/lib.rs b/backend/parsers/windmill-parser-yaml/src/lib.rs index 8a1f1bc097..5d9945cadd 100644 --- a/backend/parsers/windmill-parser-yaml/src/lib.rs +++ b/backend/parsers/windmill-parser-yaml/src/lib.rs @@ -50,7 +50,7 @@ pub fn parse_ansible_sig(inner_content: &str) -> anyhow::Result anyhow::Result anyhow::Result anyhow::Result,,...`). A newline or other config-meaningful +/// character would let a script inject arbitrary `[defaults]` directives (e.g. +/// `library`, `action_plugins`) and execute attacker-controlled code on the worker, +/// and a `,` would smuggle in an extra entry. Restrict entries to the `label@source` +/// charset so neither is possible. +pub fn validate_vault_id(value: &str) -> anyhow::Result<()> { + let is_valid = !value.is_empty() + && value + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-' | '/' | '@')); + if !is_valid { + return Err(anyhow!( + "Invalid vault_id `{value}`: expected `label@filename` using only letters, digits and the characters `.`, `_`, `-`, `/`, `@`" + )); + } + Ok(()) +} + pub fn parse_ansible_reqs( inner_content: &str, ) -> anyhow::Result<(String, Option, String)> { @@ -528,6 +547,7 @@ pub fn parse_ansible_reqs( let Yaml::String(filename) = f else { return Err(anyhow!("The elements of the vault_id field should be strings in the format: `label@filename`")); }; + validate_vault_id(filename)?; ret.vault_id.push(filename.to_string()); } } @@ -1051,4 +1071,55 @@ delegate_to_git_repo: Some("inventories/{{ env }}") ); } + + #[test] + fn test_parse_vault_id_valid() { + let p = r#" +--- +vault_id: + - dev@vault_pass_dev.txt + - prod@./secrets/prod-pass +--- +- name: Test + hosts: all +"#; + let (_, reqs, _) = parse_ansible_reqs(p).unwrap(); + assert_eq!( + reqs.unwrap().vault_id, + vec![ + "dev@vault_pass_dev.txt".to_string(), + "prod@./secrets/prod-pass".to_string() + ] + ); + } + + #[test] + fn test_parse_vault_id_rejects_newline_injection() { + let p = "---\nvault_id:\n - \"default@/tmp/wm/x\\nlibrary = /tmp/wm/evil_modules\"\n---\n- name: Test\n hosts: all\n"; + assert!(parse_ansible_reqs(p).is_err()); + } + + #[test] + fn test_parse_vault_id_rejects_comma() { + let p = r#" +--- +vault_id: + - "a@b,c@d" +--- +- name: Test + hosts: all +"#; + assert!(parse_ansible_reqs(p).is_err()); + } + + #[test] + fn test_validate_vault_id() { + assert!(validate_vault_id("default@/tmp/wm/pass").is_ok()); + assert!(validate_vault_id("dev@pass.txt").is_ok()); + assert!(validate_vault_id("").is_err()); + assert!(validate_vault_id("a@b\nlibrary = /evil").is_err()); + assert!(validate_vault_id("a@b,c@d").is_err()); + assert!(validate_vault_id("a@b c").is_err()); + assert!(validate_vault_id("a@b=c").is_err()); + } } diff --git a/backend/windmill-worker/src/ansible_executor.rs b/backend/windmill-worker/src/ansible_executor.rs index 7b0ae3b3ea..dec0623e36 100644 --- a/backend/windmill-worker/src/ansible_executor.rs +++ b/backend/windmill-worker/src/ansible_executor.rs @@ -22,7 +22,8 @@ use windmill_common::{ use windmill_queue::MiniPulledJob; use windmill_parser_yaml::{ - AnsibleRequirements, GitRepo, PreexistingAnsibleInventory, ResourceOrVariablePath, + validate_vault_id, AnsibleRequirements, GitRepo, PreexistingAnsibleInventory, + ResourceOrVariablePath, }; use windmill_queue::{append_logs, CanceledBy}; @@ -910,6 +911,11 @@ pub fn create_ansible_cfg( } if let Some(vault_ids) = reqs.as_ref().map(|r| &r.vault_id) { if !vault_ids.is_empty() { + // Defense in depth: entries are validated at parse time, but re-check here + // since they are interpolated raw into ansible.cfg (config-directive injection). + for vault_id in vault_ids { + validate_vault_id(vault_id)?; + } let password_files = vault_ids.join(","); passwords_cfg.push_str(&format!("vault_identity_list = {password_files}\n")); @@ -1799,4 +1805,31 @@ mod tests { assert!(validate_relative_path("", "playbook").is_err()); assert!(validate_relative_path(" ", "playbook").is_err()); } + + #[test] + fn test_create_ansible_cfg_writes_valid_vault_id() { + let dir = tempfile::tempdir().unwrap(); + let job_dir = dir.path().to_str().unwrap(); + let reqs = AnsibleRequirements { + vault_id: vec!["dev@vault_pass.txt".to_string()], + ..Default::default() + }; + create_ansible_cfg(Some(&reqs), job_dir, false).unwrap(); + let cfg = std::fs::read_to_string(dir.path().join("ansible.cfg")).unwrap(); + assert!(cfg.contains("vault_identity_list = dev@vault_pass.txt")); + assert!(!cfg.contains("library")); + } + + #[test] + fn test_create_ansible_cfg_rejects_vault_id_injection() { + let dir = tempfile::tempdir().unwrap(); + let job_dir = dir.path().to_str().unwrap(); + let reqs = AnsibleRequirements { + vault_id: vec!["default@/tmp/wm/x\nlibrary = /tmp/wm/evil_modules".to_string()], + ..Default::default() + }; + // Defense-in-depth boundary: a poisoned entry must error before any config is written. + assert!(create_ansible_cfg(Some(&reqs), job_dir, false).is_err()); + assert!(!dir.path().join("ansible.cfg").exists()); + } }