From 0dd131b38074423de5c769cae478c6ea0ae365c2 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:50:32 +0200 Subject: [PATCH] fix: compare fork secret variables by decrypted value (#11952) * fix: compare fork secret variables by decrypted value * test: restore MIN_VERSION even when the secret compare test fails * fix: re-evaluate cached variable diffs once after the secret compare fix * refactor: decrypt already-loaded secret rows when comparing variables * docs: state the authorization contract of decrypt_secret_value * fix: compare external-backend secrets by their resolved values --------- Co-authored-by: Ruben Fiszel --- ...29ac1a2714b78cc88969604dbd6c44984831f.json | 12 ++ ...666e83c6ccdea5087eb194d1de17086c58952.json | 12 ++ ...83cbaf7676cbed2f0c3a433699578da88e8ae.json | 20 ++++ ...t_cached_variable_workspace_diffs.down.sql | 1 + ...set_cached_variable_workspace_diffs.up.sql | 3 + .../tests/workspace_comparison.rs | 111 ++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 25 +++- backend/windmill-common/src/variables.rs | 44 +++---- 8 files changed, 202 insertions(+), 26 deletions(-) create mode 100644 backend/.sqlx/query-e1d7ca4dfbc5eeb6eb60e48a13029ac1a2714b78cc88969604dbd6c44984831f.json create mode 100644 backend/.sqlx/query-e57039a483e96afa69f96e36be7666e83c6ccdea5087eb194d1de17086c58952.json create mode 100644 backend/.sqlx/query-ecd08125821a21842bad7d328ea83cbaf7676cbed2f0c3a433699578da88e8ae.json create mode 100644 backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.down.sql create mode 100644 backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.up.sql diff --git a/backend/.sqlx/query-e1d7ca4dfbc5eeb6eb60e48a13029ac1a2714b78cc88969604dbd6c44984831f.json b/backend/.sqlx/query-e1d7ca4dfbc5eeb6eb60e48a13029ac1a2714b78cc88969604dbd6c44984831f.json new file mode 100644 index 0000000000..90a3f9c7f2 --- /dev/null +++ b/backend/.sqlx/query-e1d7ca4dfbc5eeb6eb60e48a13029ac1a2714b78cc88969604dbd6c44984831f.json @@ -0,0 +1,12 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM workspace_diff WHERE fork_workspace_id = 'wm-fork-secrets'", + "describe": { + "columns": [], + "parameters": { + "Left": [] + }, + "nullable": [] + }, + "hash": "e1d7ca4dfbc5eeb6eb60e48a13029ac1a2714b78cc88969604dbd6c44984831f" +} diff --git a/backend/.sqlx/query-e57039a483e96afa69f96e36be7666e83c6ccdea5087eb194d1de17086c58952.json b/backend/.sqlx/query-e57039a483e96afa69f96e36be7666e83c6ccdea5087eb194d1de17086c58952.json new file mode 100644 index 0000000000..c9507b7f22 --- /dev/null +++ b/backend/.sqlx/query-e57039a483e96afa69f96e36be7666e83c6ccdea5087eb194d1de17086c58952.json @@ -0,0 +1,12 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO workspace_diff\n (source_workspace_id, fork_workspace_id, path, kind, ahead, behind, has_changes)\n VALUES\n ('test-workspace', 'wm-fork-secrets', 'u/test-user/same', 'variable', 1, 1, NULL),\n ('test-workspace', 'wm-fork-secrets', 'u/test-user/different', 'variable', 1, 1, NULL)", + "describe": { + "columns": [], + "parameters": { + "Left": [] + }, + "nullable": [] + }, + "hash": "e57039a483e96afa69f96e36be7666e83c6ccdea5087eb194d1de17086c58952" +} diff --git a/backend/.sqlx/query-ecd08125821a21842bad7d328ea83cbaf7676cbed2f0c3a433699578da88e8ae.json b/backend/.sqlx/query-ecd08125821a21842bad7d328ea83cbaf7676cbed2f0c3a433699578da88e8ae.json new file mode 100644 index 0000000000..db600a495e --- /dev/null +++ b/backend/.sqlx/query-ecd08125821a21842bad7d328ea83cbaf7676cbed2f0c3a433699578da88e8ae.json @@ -0,0 +1,20 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT COUNT(DISTINCT value) FROM variable WHERE path = 'u/test-user/same'", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "count", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [] + }, + "nullable": [ + null + ] + }, + "hash": "ecd08125821a21842bad7d328ea83cbaf7676cbed2f0c3a433699578da88e8ae" +} diff --git a/backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.down.sql b/backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.down.sql new file mode 100644 index 0000000000..d2f607c5b8 --- /dev/null +++ b/backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.down.sql @@ -0,0 +1 @@ +-- Add down migration script here diff --git a/backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.up.sql b/backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.up.sql new file mode 100644 index 0000000000..c531389793 --- /dev/null +++ b/backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.up.sql @@ -0,0 +1,3 @@ +-- Secrets compared by ciphertext were cached as changed even when both sides held the same value, +-- and a cached verdict is trusted until the next write. Re-evaluate every variable row once. +UPDATE workspace_diff SET has_changes = NULL WHERE kind = 'variable'; diff --git a/backend/windmill-api-integration-tests/tests/workspace_comparison.rs b/backend/windmill-api-integration-tests/tests/workspace_comparison.rs index a8ab80c7b7..c3cb9ff190 100644 --- a/backend/windmill-api-integration-tests/tests/workspace_comparison.rs +++ b/backend/windmill-api-integration-tests/tests/workspace_comparison.rs @@ -2635,3 +2635,114 @@ async fn test_compare_null_description_equals_empty(db: Pool) -> anyho Ok(()) } + +/// Runs its closure when dropped, so process-wide state is put back even when an assertion +/// fails. +struct OnDrop(F); +impl Drop for OnDrop { + fn drop(&mut self) { + (self.0)() + } +} + +/// Secrets are encrypted with a fresh nonce on every write, so a secret holding the same value on +/// both sides must compare by plaintext, not ciphertext. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_compare_secret_variables_by_value(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let client = windmill_api_client::create_client( + &format!("http://localhost:{port}"), + "SECRET_TOKEN".to_string(), + ); + let base_url = format!("http://localhost:{port}/api"); + + let resp = client + .client() + .post(&format!( + "{base_url}/w/test-workspace/workspaces/create_fork" + )) + .json(&json!({ "id": "wm-fork-secrets", "name": "Secrets Fork" })) + .send() + .await?; + assert!( + resp.status().is_success(), + "fork creation failed: {}", + resp.status() + ); + + // Secrets are written in CBC, which is deterministic, until every worker reads AES-GCM. + use windmill_common::min_version::{MIN_VERSION, MIN_VERSION_SUPPORTS_AEAD_SECRETS}; + let before = MIN_VERSION.load_full(); + let _restore = OnDrop(move || MIN_VERSION.store(before.clone())); + MIN_VERSION.store(std::sync::Arc::new( + MIN_VERSION_SUPPORTS_AEAD_SECRETS.version().clone(), + )); + + for (ws, path, value) in [ + ("test-workspace", "u/test-user/same", "s3cret"), + ("wm-fork-secrets", "u/test-user/same", "s3cret"), + ("test-workspace", "u/test-user/different", "one"), + ("wm-fork-secrets", "u/test-user/different", "two"), + ] { + let resp = client + .client() + .post(&format!("{base_url}/w/{ws}/variables/create")) + .json(&json!({ "path": path, "value": value, "is_secret": true, "description": "" })) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + } + let stored = sqlx::query_scalar!( + "SELECT COUNT(DISTINCT value) FROM variable WHERE path = 'u/test-user/same'" + ) + .fetch_one(&db) + .await?; + assert_eq!( + stored, + Some(2), + "the same secret must be stored as two different ciphertexts" + ); + + sqlx::query!("DELETE FROM workspace_diff WHERE fork_workspace_id = 'wm-fork-secrets'") + .execute(&db) + .await?; + sqlx::query!( + "INSERT INTO workspace_diff + (source_workspace_id, fork_workspace_id, path, kind, ahead, behind, has_changes) + VALUES + ('test-workspace', 'wm-fork-secrets', 'u/test-user/same', 'variable', 1, 1, NULL), + ('test-workspace', 'wm-fork-secrets', 'u/test-user/different', 'variable', 1, 1, NULL)" + ) + .execute(&db) + .await?; + + let comparison: serde_json::Value = client + .client() + .get(&format!( + "{base_url}/w/test-workspace/workspaces/compare/wm-fork-secrets" + )) + .send() + .await? + .json() + .await?; + let diffs = comparison["diffs"].as_array().unwrap(); + let listed = |path: &str| { + diffs + .iter() + .any(|d| d["path"] == path && d["kind"] == "variable") + }; + + assert!( + !listed("u/test-user/same"), + "equal secrets must compare as unchanged: {comparison}" + ); + assert!( + listed("u/test-user/different"), + "different secrets must compare as changed: {comparison}" + ); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index ac1d0d1300..e2027c3e9b 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -39,7 +39,7 @@ use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE; use windmill_common::query_builders::{render_db_quoted_identifier, DbType}; use windmill_common::users::username_to_permissioned_as; use windmill_common::variables::{ - crypt_from_key_with_suffix, decrypt, encrypt, WORKSPACE_CRYPT_CACHE, + crypt_from_key_with_suffix, decrypt, decrypt_secret_value, encrypt, WORKSPACE_CRYPT_CACHE, }; use windmill_common::worker::{to_raw_value, CLOUD_HOSTED}; use windmill_common::workspaces::GitRepositorySettings; @@ -76,7 +76,9 @@ use hyper::StatusCode; use serde::{Deserialize, Serialize}; use sqlx::{FromRow, Postgres, Row, Transaction}; use windmill_common::oauth2::InstanceEvent; -use windmill_common::secret_backend::{get_secret_backend, is_vault_backend_configured}; +use windmill_common::secret_backend::{ + get_secret_backend, is_external_stored_value, is_vault_backend_configured, +}; use windmill_common::utils::not_found_if_none; lazy_static::lazy_static! { @@ -14524,11 +14526,22 @@ async fn compare_two_variables( // Check metadata differences if let (Some(source), Some(target)) = (&source_variable, &target_variable) { - if source.is_secret != target.is_secret - || source.value != target.value - || source.description != target.description - { + if source.is_secret != target.is_secret || source.description != target.description { has_changes = true; + } else if source.value != target.value + || (source.is_secret && is_external_stored_value(&source.value)) + { + // A secret's ciphertext is randomized on every write, so equal secrets are compared + // by plaintext, each decrypted with its own workspace's key. An external backend + // reference is the same text on both sides but names a per-workspace entry. + has_changes = !source.is_secret + || !matches!( + tokio::try_join!( + decrypt_secret_value(db, source_workspace_id, path, source.value.clone()), + decrypt_secret_value(db, fork_workspace_id, path, target.value.clone()), + ), + Ok((a, b)) if a == b + ); } } else if source_variable.is_some() || target_variable.is_some() { // The variable exists in one of source or target, but not the other, this is considered as a change diff --git a/backend/windmill-common/src/variables.rs b/backend/windmill-common/src/variables.rs index 82039d71f5..512bd555f0 100644 --- a/backend/windmill-common/src/variables.rs +++ b/backend/windmill-common/src/variables.rs @@ -465,28 +465,32 @@ pub async fn get_secret_value_as_admin( ))); }; - let r = if variable.is_secret { - let value = variable.value; - if !value.is_empty() { - if is_external_stored_value(&value) { - get_secret_value(db, w_id, &variable.path, &value).await? - } else { - let mc = build_crypt(db, w_id).await?; - decrypt(&mc, value).map_err(|e| { - crate::error::Error::internal_err(format!( - "Error decrypting variable {}: {}", - variable.path, e - )) - })? - } - } else { - "".to_string() - } + if variable.is_secret { + decrypt_secret_value(db, w_id, &variable.path, variable.value).await } else { - variable.value - }; + Ok(variable.value) + } +} - Ok(r) +/// Plaintext of a secret variable's stored `value`, read from the external secret backend when it +/// holds a reference to one. Bypasses variable permissions: callers MUST have authorized the +/// reader before returning the plaintext to them. +pub async fn decrypt_secret_value( + db: &DB, + w_id: &str, + path: &str, + value: String, +) -> crate::error::Result { + if value.is_empty() { + Ok(value) + } else if is_external_stored_value(&value) { + get_secret_value(db, w_id, path, &value).await + } else { + let mc = build_crypt(db, w_id).await?; + decrypt(&mc, value).map_err(|e| { + crate::error::Error::internal_err(format!("Error decrypting variable {}: {}", path, e)) + }) + } } /// Marks a workspace setting's secret written into the settings document encrypted with the