From 92b7f375a90de2f78565ca06a13c79ff04eda44d Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sun, 12 Jul 2026 10:19:20 +0200 Subject: [PATCH] fix: replicate all secrets on fork when external backend is configured (#10060) * fix: replicate all secrets on fork with external backend (WIN-2161) Co-Authored-By: Claude Opus 4.8 * test: add Azure KV fork secret-replication reproduction (WIN-2161) Co-Authored-By: Claude Opus 4.8 * style: condense clone_variables invariant comment (WIN-2161) Co-Authored-By: Claude Opus 4.8 * test: drive real create_fork handler in Azure KV repro (WIN-2161) Replace the windmill-common test that mirrored clone_variables' loop with an end-to-end test in windmill-api-integration-tests that exercises the real migration, create_fork and variable-read endpoints against a local Azure KV emulator. Verified it fails (404 "not found in Azure Key Vault") without the fix and passes with it; unique per-run ids keep it robust to the emulator's persistent state. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- ...4f6e02030d8581a7e8c1f0673c7f6dae78fb3.json | 22 +++ ...819520a2ba987fd29859845c2177563ab8cfb.json | 28 --- .../tests/fork_secret_replication_azure.rs | 161 ++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 31 ++-- 4 files changed, 198 insertions(+), 44 deletions(-) create mode 100644 backend/.sqlx/query-b6de5fbd5fa89e9a8ab8a61982d4f6e02030d8581a7e8c1f0673c7f6dae78fb3.json delete mode 100644 backend/.sqlx/query-e4a46d47aee96473a2bd0f1dbdc819520a2ba987fd29859845c2177563ab8cfb.json create mode 100644 backend/windmill-api-integration-tests/tests/fork_secret_replication_azure.rs diff --git a/backend/.sqlx/query-b6de5fbd5fa89e9a8ab8a61982d4f6e02030d8581a7e8c1f0673c7f6dae78fb3.json b/backend/.sqlx/query-b6de5fbd5fa89e9a8ab8a61982d4f6e02030d8581a7e8c1f0673c7f6dae78fb3.json new file mode 100644 index 0000000000..d05cc5cdd4 --- /dev/null +++ b/backend/.sqlx/query-b6de5fbd5fa89e9a8ab8a61982d4f6e02030d8581a7e8c1f0673c7f6dae78fb3.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT path FROM variable\n WHERE workspace_id = $1 AND is_secret = true AND value != ''", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "b6de5fbd5fa89e9a8ab8a61982d4f6e02030d8581a7e8c1f0673c7f6dae78fb3" +} diff --git a/backend/.sqlx/query-e4a46d47aee96473a2bd0f1dbdc819520a2ba987fd29859845c2177563ab8cfb.json b/backend/.sqlx/query-e4a46d47aee96473a2bd0f1dbdc819520a2ba987fd29859845c2177563ab8cfb.json deleted file mode 100644 index 18c75f5722..0000000000 --- a/backend/.sqlx/query-e4a46d47aee96473a2bd0f1dbdc819520a2ba987fd29859845c2177563ab8cfb.json +++ /dev/null @@ -1,28 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT path, value FROM variable\n WHERE workspace_id = $1 AND is_secret = true AND value != ''", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "path", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "value", - "type_info": "Varchar" - } - ], - "parameters": { - "Left": [ - "Text" - ] - }, - "nullable": [ - false, - false - ] - }, - "hash": "e4a46d47aee96473a2bd0f1dbdc819520a2ba987fd29859845c2177563ab8cfb" -} diff --git a/backend/windmill-api-integration-tests/tests/fork_secret_replication_azure.rs b/backend/windmill-api-integration-tests/tests/fork_secret_replication_azure.rs new file mode 100644 index 0000000000..19682de70a --- /dev/null +++ b/backend/windmill-api-integration-tests/tests/fork_secret_replication_azure.rs @@ -0,0 +1,161 @@ +//! End-to-end regression test for WIN-2161. +//! +//! Reproduces, through real product code, the state after a database-to-external +//! migration: a secret that was created under the database backend and then +//! *migrated* to an external backend (Azure Key Vault). Migration writes the +//! plaintext to the store but +//! leaves the encrypted ciphertext in `variable.value` (it never rewrites it to +//! a `$azure_kv:` marker). The bug: `clone_variables` only replicated +//! marker-valued secrets, so forking left the migrated secret unreplicated and +//! reads in the fork failed with "not found in Azure Key Vault". +//! +//! This drives the real `/migrate_secrets_to_azure_kv`, `/create_fork` and +//! `variables/get_value` endpoints against a local Azure Key Vault emulator +//! (lowkey-vault), which the `AzureKeyVaultBackend` talks to via its +//! static-token / self-signed-cert emulator mode. +//! +//! Run it: +//! ```bash +//! podman run -d --name lowkey -p 8443:8443 \ +//! -e LOWKEY_ARGS="--LOWKEY_VAULT_NAMES=default" \ +//! docker.io/nagyesta/lowkey-vault:7.3.0 +//! +//! RUN_AZURE_KV_TESTS=1 cargo test -p windmill-api-integration-tests \ +//! --features private,enterprise --test fork_secret_replication_azure -- --nocapture +//! ``` + +#[cfg(all(feature = "private", feature = "enterprise"))] +mod azure_fork { + use serde_json::json; + use sqlx::{Pool, Postgres}; + use windmill_common::variables::{build_crypt, encrypt}; + use windmill_test_utils::*; + + fn client() -> reqwest::Client { + reqwest::Client::new() + } + + fn authed(builder: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + builder.header("Authorization", "Bearer SECRET_TOKEN") + } + + fn vault_url() -> String { + std::env::var("AZURE_KV_URL").unwrap_or_else(|_| "https://localhost:8443".to_string()) + } + + /// The Azure settings for the emulator: a static token switches the backend + /// into emulator mode (no Entra ID, self-signed certs accepted). + fn azure_settings() -> serde_json::Value { + json!({ + "vault_url": vault_url(), + "tenant_id": "emulator-tenant", + "client_id": "emulator-client", + "token": "emulator-token", + }) + } + + #[sqlx::test(migrations = "../migrations", fixtures("base"))] + async fn migrated_secret_is_replicated_on_fork(db: Pool) -> anyhow::Result<()> { + if std::env::var("RUN_AZURE_KV_TESTS").as_deref() != Ok("1") { + eprintln!("skipping: set RUN_AZURE_KV_TESTS=1 and start lowkey-vault to run"); + return Ok(()); + } + initialize_tracing().await; + + // The Azure KV emulator persists across runs; derive unique names per run + // so a secret written by a previous run can't mask a regression. + let suffix = uuid::Uuid::new_v4().simple().to_string(); + let short = &suffix[..8]; + let source_ws = "test-workspace"; + let path = format!("u/test-user/db_password_{short}"); + let path = path.as_str(); + let plaintext = "s3cr3t-value"; + + let ciphertext = { + let mc = build_crypt(&db, source_ws).await?; + encrypt(&mc, plaintext) + }; + sqlx::query( + "INSERT INTO variable (workspace_id, path, value, is_secret, description, extra_perms) + VALUES ($1, $2, $3, true, '', '{}')", + ) + .bind(source_ws) + .bind(path) + .bind(&ciphertext) + .execute(&db) + .await?; + + sqlx::query( + "INSERT INTO global_settings (name, value) VALUES ('secret_backend', $1) + ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value", + ) + .bind(json!({ + "type": "AzureKeyVault", + "vault_url": vault_url(), + "tenant_id": "emulator-tenant", + "client_id": "emulator-client", + "token": "emulator-token", + })) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let resp = authed(client().post(format!( + "http://localhost:{port}/api/settings/migrate_secrets_to_azure_kv" + ))) + .json(&azure_settings()) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 200, "migrate_secrets_to_azure_kv failed: {body}"); + let report: serde_json::Value = serde_json::from_str(&body)?; + assert!( + report["migrated_count"].as_i64().unwrap_or(0) >= 1, + "expected at least one migrated secret: {report}" + ); + + // Assert the source resolves before forking, so a fork-read failure is + // attributable to replication rather than a broken seed. + let resp = authed(client().get(format!( + "http://localhost:{port}/api/w/{source_ws}/variables/get_value/{path}" + ))) + .send() + .await?; + assert_eq!(resp.status(), 200, "source read: {}", resp.text().await?); + assert_eq!(resp.json::().await?, plaintext); + + let fork_ws = format!("wm-fork-az{short}"); + let fork_ws = fork_ws.as_str(); + let resp = authed(client().post(format!( + "http://localhost:{port}/api/w/{source_ws}/workspaces/create_fork" + ))) + .json(&json!({ "id": fork_ws, "name": "Azure Fork Test" })) + .send() + .await?; + assert_eq!(resp.status(), 200, "create_fork: {}", resp.text().await?); + + // The fork resolves the secret only if it was replicated under the fork's + // own workspace-id key in the external store. + let resp = authed(client().get(format!( + "http://localhost:{port}/api/w/{fork_ws}/variables/get_value/{path}" + ))) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 200, + "forked secret must resolve, got {status}: {body}" + ); + assert_eq!( + serde_json::from_str::(&body)?, + plaintext, + "fork should return the replicated plaintext" + ); + + Ok(()) + } +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 2c28a6729a..ba359cbffb 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -71,9 +71,7 @@ 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_external_stored_value, is_vault_backend_configured, -}; +use windmill_common::secret_backend::{get_secret_backend, is_vault_backend_configured}; use windmill_common::utils::not_found_if_none; lazy_static::lazy_static! { @@ -4703,15 +4701,13 @@ async fn clone_variables( .execute(&mut **tx) .await?; - // With an external secret backend (Vault / Azure KV / AWS SM), the copied - // `value` is only a `$vault:`/`$azure_kv:`/`$aws_sm:` marker: the actual - // secret lives in the external store under a key derived from - // (workspace_id, path). The row copy above therefore leaves the fork's - // markers pointing at keys that don't exist — replicate each secret under - // the fork's workspace id. + // With an external backend the secret lives in the store under (workspace_id, + // path), so the row copy above leaves the fork pointing at keys that don't + // exist. Replicate every secret, not just marker-valued ones: migration writes + // to the store without rewriting `value` to a `$...:` marker. if is_vault_backend_configured(db).await? { let secret_variables = sqlx::query!( - "SELECT path, value FROM variable + "SELECT path FROM variable WHERE workspace_id = $1 AND is_secret = true AND value != ''", target_workspace_id, ) @@ -4719,10 +4715,7 @@ async fn clone_variables( .await?; let backend = get_secret_backend(db).await?; - for variable in secret_variables - .into_iter() - .filter(|v| is_external_stored_value(&v.value)) - { + for variable in secret_variables { match backend .get_secret(source_workspace_id, &variable.path) .await @@ -5810,8 +5803,14 @@ async fn create_workspace_fork( .await?; // Clone all data from the parent workspace using Rust implementation - if let Err(e) = - clone_workspace_data(&mut tx, &db, &parent_workspace_id, &forked_id, &authed.email).await + if let Err(e) = clone_workspace_data( + &mut tx, + &db, + &parent_workspace_id, + &forked_id, + &authed.email, + ) + .await { // A genuine `\u0000` in a source `json` value (`app_version.value` / // `flow_version.schema`) aborts the clone when it is re-encoded to jsonb: