diff --git a/backend/windmill-api/src/dbt.rs b/backend/windmill-api/src/dbt.rs index ccdd189f66..2fd4e46679 100644 --- a/backend/windmill-api/src/dbt.rs +++ b/backend/windmill-api/src/dbt.rs @@ -28,9 +28,9 @@ async fn get_warehouse( Path((w_id, name)): Path<(String, String)>, ) -> Result> { // Scoped to a running DBT job, and the reason it must stay that way: the - // response carries the warehouse's credentials. A dbt job already holds them - // in its rendered `profiles.yml`, so serving them changes nothing for it — - // but every script job's token carries a job id too, and any other language + // response carries the warehouse's credentials. A dbt job connects with them + // and so already holds them, so serving them changes nothing for it — but + // every script job's token carries a job id too, and any other language // asking for them would be reading a credential it was never given. // In no-auth mode every request is the synthetic superadmin and carries no // job, so the scoping below has nothing to check. Refusing there would make @@ -61,7 +61,11 @@ async fn get_warehouse( )) })?; let resource_type = warehouse_resource_type(&db, &w_id, &resource_path).await?; - return Ok(Json(DbtWarehouseConnection { value, target, resource_type })); + return Ok(Json(DbtWarehouseConnection { + value, + target, + resource_type, + })); } return Err(Error::BadRequest( "this route resolves a dbt warehouse for a running job and needs a job token" @@ -104,7 +108,11 @@ async fn get_warehouse( )) })?; let resource_type = warehouse_resource_type(&db, &w_id, &resource_path).await?; - Ok(Json(DbtWarehouseConnection { value, target, resource_type })) + Ok(Json(DbtWarehouseConnection { + value, + target, + resource_type, + })) } /// A warehouse resource's type, which decides whether its value is translated @@ -118,7 +126,11 @@ async fn warehouse_resource_type(db: &DB, w_id: &str, path: &str) -> Result bool { @@ -93,15 +97,35 @@ impl ProfileSecrets { } /// One target key's value: hidden when the key names a credential, inline - /// otherwise. An empty value is always inline — dbt scrubs by substring, and - /// an empty secret matches between every pair of characters in the log. + /// otherwise. fn field(&mut self, key: &str, value: &str) -> ProfileValue { - if is_credential_key(key) && !value.is_empty() { + self.leaf(is_credential_key(key), value) + } + + /// A value whose classification the caller already knows, from its own key + /// or an enclosing one. + fn leaf(&mut self, secret: bool, value: &str) -> ProfileValue { + if secret && Self::hideable(value) { self.hide(value) } else { quoted(value) } } + + /// Whether standing in for this value is safe. + /// + /// An EMPTY value never is: dbt redacts by substring, and an empty one + /// matches between every pair of characters in the log. + /// + /// Neither is a Jinja expression, which NAMES a credential rather than being + /// one — a `dbt_profile` block is pasted from a working `profiles.yml`, + /// where `password: "{{ env_var('PGPASSWORD') }}"` is the ordinary shape, + /// resolved against the descriptor's `env`. dbt renders a profile value once + /// and does not re-render what `env_var()` returns, so standing in for that + /// text would hand the adapter the template instead of the password. + fn hideable(value: &str) -> bool { + !value.is_empty() && !value.contains("{{") && !value.contains("{%") + } } /// The per-adapter facts, so each adapter states them together and a new one @@ -873,7 +897,7 @@ pub fn render_dbt_profile( if (k == schema_key && schema_override.is_some()) || (k == "threads" && threads.is_some()) { continue; } - emit_entry(&mut yaml, 6, k, v, &mut secrets); + emit_entry(&mut yaml, 6, k, v, false, &mut secrets); } if root_certificate_pem.is_some() { yaml.push_str(&format!( @@ -907,19 +931,31 @@ pub fn render_dbt_profile( /// Emit one target key, nesting as deep as the value goes — an adapter's credential can be /// a mapping (bigquery's `keyfile_json`) or a list. Keys are quoted like values: one nothing /// here enumerates is as free-form as a password. -fn emit_entry(out: &mut String, indent: usize, key: &str, v: &Value, secrets: &mut ProfileSecrets) { +/// +/// `enclosing_secret` is what a key ABOVE this one decided. Everything under a key naming a +/// credential is credential material, whatever it is called — an `oauth_credentials` mapping +/// spells its own keys, and Windmill has no list to check them against. +fn emit_entry( + out: &mut String, + indent: usize, + key: &str, + v: &Value, + enclosing_secret: bool, + secrets: &mut ProfileSecrets, +) { out.push_str(&format!("{}{}:", " ".repeat(indent), yaml_scalar(key))); - emit_value(out, indent, v, is_credential_key(key), secrets); + emit_value( + out, + indent, + v, + enclosing_secret || is_credential_key(key), + secrets, + ); } /// The value half, after `key:`. An empty collection is emitted INLINE: a block with no /// children reads back as `null`, so `extensions: []` would reach the adapter as a missing /// value rather than the empty list dbt was handed. -/// -/// `secret` says whether the key this value hangs off names a credential. A list inherits -/// it, since its items share that one key; a mapping does not, because each of its own keys -/// answers for itself — a `keyfile_json` holds a private key beside a project id, and dbt -/// redacts a secret's value wherever it appears in the run's output. fn emit_value( out: &mut String, indent: usize, @@ -938,7 +974,7 @@ fn emit_value( } out.push('\n'); for (k, v) in kept { - emit_entry(out, indent + 2, k, v, secrets); + emit_entry(out, indent + 2, k, v, secret, secrets); } } Value::Array(items) => { @@ -954,9 +990,7 @@ fn emit_value( emit_value(out, indent + 2, item, secret, secrets); } } - Value::String(s) if secret && !s.is_empty() => { - out.push_str(&format!(" {}\n", secrets.hide(s).render())) - } + Value::String(s) => out.push_str(&format!(" {}\n", secrets.leaf(secret, s).render())), _ => out.push_str(&format!(" {}\n", yaml_value(v))), } } @@ -1535,12 +1569,14 @@ mod tests { } // A `dbt_profile` block is written for an adapter Windmill may know nothing - // about, so the credential is recognized by key name — including nested, where - // a mapping's own keys answer for themselves rather than inheriting. + // about, so the credential is recognized by key name, and everything under + // such a key with it. #[test] fn a_dbt_profile_hides_its_credential_keys() { let r = json!({"type": "trino", "host": "trino.internal", "schema": "analytics", - "user": "u", "password": "pw", "http_headers": {"x-api-key": "k"}, + "user": "u", "password": "pw", + "http_headers": {"x-api-key": "k", "X-Trace": "on"}, + "oauth_credentials": {"client": "c"}, "keyfile_json": {"project_id": "proj", "private_key": "pem"}}); let p = render_dbt_profile( &DbtAdapter::from_dbt_type("trino").unwrap(), @@ -1554,15 +1590,44 @@ mod tests { .unwrap(); let mut hidden: Vec<&str> = p.env.iter().map(|(_, v)| v.as_str()).collect(); hidden.sort(); - assert_eq!(hidden, vec!["k", "pem", "pw"], "{}", p.yaml); + // `client` for the enclosing `oauth_credentials`; `X-Trace` names nothing + // and its enclosing key names nothing either. + assert_eq!(hidden, vec!["c", "k", "pem", "pw"], "{}", p.yaml); let parsed: serde_yml::Value = serde_yml::from_str(&p.yaml).unwrap(); let target = &parsed["wm"]["outputs"]["prod"]; assert_eq!(target["host"].as_str(), Some("trino.internal")); assert_eq!(target["user"].as_str(), Some("u")); + assert_eq!(target["http_headers"]["X-Trace"].as_str(), Some("on")); assert_eq!(target["keyfile_json"]["project_id"].as_str(), Some("proj")); assert_eq!(p.schema.as_deref(), Some("analytics")); } + // The block is pasted from a working `profiles.yml`, where a credential is + // ordinarily an `env_var()` resolved against the descriptor's `env`. dbt + // renders a profile value once, so standing in for that expression would + // hand the adapter the template text instead of the password. + #[test] + fn a_dbt_profile_leaves_an_env_var_expression_for_dbt_to_render() { + let r = json!({"type": "clickhouse", "host": "ch.internal", + "password": "{{ env_var('CH_PASSWORD') }}"}); + let p = render_dbt_profile( + &KnownAdapter::Clickhouse.into(), + r.as_object().unwrap(), + "wm", + "prod", + None, + None, + std::path::Path::new("/tmp/p"), + ) + .unwrap(); + assert!(p.env.is_empty(), "{:?}", p.env); + let parsed: serde_yml::Value = serde_yml::from_str(&p.yaml).unwrap(); + assert_eq!( + parsed["wm"]["outputs"]["prod"]["password"].as_str(), + Some("{{ env_var('CH_PASSWORD') }}") + ); + } + // A RUNTIME check, because one dbt executor serves every adapter. A refactor // reaching for a bare `LICENSE_KEY_VALID` would silently let CE through, since // the OSS variant initializes it to `true`. diff --git a/docs/dbt-runtime.md b/docs/dbt-runtime.md index 9f7b557882..a33a33058c 100644 --- a/docs/dbt-runtime.md +++ b/docs/dbt-runtime.md @@ -30,7 +30,7 @@ the dominant way dbt is orchestrated today. | 5 | Project storage | The project is the script's module bundle; nothing is cloned. See "Where the dbt project lives" | | 6 | Multiple run configs | Per-run `select` on one script; N scripts means N projects | | 7 | Run-time `select` | Descriptor default plus run-arg override | -| 8 | Credentials | Workspace warehouses, plus `profiles.yml` passthrough. A descriptor never names a resource. A rendered profile names its credentials through `env_var()` and carries none. See below | +| 8 | Credentials | Workspace warehouses, plus `profiles.yml` passthrough. A descriptor never names a resource. A rendered profile names its credentials through `env_var()` rather than carrying them. See below | | 9 | Adapter mappings | postgres, redshift, mysql, snowflake, bigquery, databricks translate from their Windmill resource; **every** adapter dbt has is reachable from a `dbt_profile` resource, or the project's own `profiles.yml` | | 10 | Private repo auth | Not applicable: the project is synced, not fetched | | 11 | Asset kind | `dbt:////` — keyed on the relation, not on dbt's node id. See below | @@ -359,20 +359,43 @@ value is replaced with `*****` wherever dbt prints it, which is what Windmill streams into the job log. A plainly named variable would be worth nothing: the project would read it back with one `env_var()`. -Four things this had to get right, each of which quietly undoes it otherwise. +Five things this had to get right, each of which quietly undoes it otherwise. -**Only credentials go through a variable.** dbt redacts a secret's VALUE from -every line it prints, so routing a coordinate through one redacts the run: a -`user` of `postgres` turns `Registered adapter: postgres=1.11.0` into -`Registered adapter: *****=1.11.0`, and a `schema` of `public` would blank out -half the log. A translated resource knows its own credential fields; a -`dbt_profile` block is written for an adapter Windmill may know nothing about -(decision 24), so there it is by key name — `password`, `token`, `secret`, -`private_key`, `passphrase`, `credential`, `api_key`, `access_key`, matched as a -substring with `-` folded to `_`, so `client_secret` and an `http_headers` -`x-api-key` are covered. Nested, each mapping key answers for itself rather than -inheriting: a `keyfile_json` holds a private key beside a project id. The rule -errs toward leaving a value inline, which is where it already was. +**Only credentials go through a variable, and hiding everything is not the safer +choice.** dbt redacts a secret's VALUE from every line it emits — the console +log Windmill streams to the job, *and* the JSON event log Windmill parses for +per-model status and relation names. Registering the string `public` as a secret +does not merely make the log ugly: every relation whose name contains it is +recorded as `*****`, so the run succeeds while the asset graph fills with +nonsense. Measured, not feared — a `user` of `postgres` turns +`Registered adapter: postgres=1.11.0` into `Registered adapter: *****=1.11.0`, +and a schema registered as a secret vanishes from all 64 places it appears in the +JSON log. Fail-closed is the wrong direction here. + +So: a translated resource knows exactly which of its fields are credentials, +and there the file provably carries none. A `dbt_profile` block is written for an +adapter Windmill may know nothing about (decision 24), so there the rule is by +key name — `password`, `token`, `secret`, `private_key`, `passphrase`, +`credential`, `api_key`, `access_key`, `authorization`, matched as a substring +with `-` folded to `_`, so `client_secret` and an `http_headers` `x-api-key` are +covered — and everything nested under such a key goes with it, since an +`oauth_credentials` mapping spells its own keys and there is no list to check +them against. `authorization` rather than `auth`, because several adapters spell +an `authenticator` naming a METHOD and redacting `oauth` from a whole run is the +failure above. + +That rule is not exhaustive over an open adapter set, and it is not claimed to +be: a key nobody recognized stays exactly where it already was, inline. The +guarantee is the translated path; the block path is a large reduction whose +boundary is a list one line long to extend. + +**A value that is already an `env_var()` expression stays inline.** A +`dbt_profile` block is pasted from a working `profiles.yml`, where +`password: "{{ env_var('PGPASSWORD') }}"` is the ordinary shape, resolved against +the descriptor's `env`. dbt renders a profile value ONCE and does not re-render +what `env_var()` returns, so standing in for that text would hand the adapter the +template instead of the password — and there is nothing to hide either way, since +such a value names a credential rather than being one. **The names are a fresh nonce per render.** `packages.yml` is rendered under the same secret context as `profiles.yml`, on every dbt invocation and not only @@ -385,12 +408,12 @@ descriptor's `env` and the script's environment variables, alongside the while a project's own `DBT_ENV_SECRET_*` package token, outside Windmill's namespace, still works. -**Run identity moved with them.** `profile_digest` is a one-way digest of the -resolved connection, and the connection is no longer all in the text, so the -values are hashed beside it — otherwise a resource repointed at another +**Run identity covers what the text no longer states.** `profile_digest` is a +one-way digest of the resolved connection, so the credential values are hashed +beside the rendered text — on the text alone, a resource repointed at another warehouse with the same host and database names would present the identity a retry saved its failures against. The nonce is normalized out of both halves for -the same reason the job's own token already was: it belongs to the attempt. +the same reason the job's own token is: it belongs to the attempt. **A project's own `profiles.yml` is passed through untouched.** It is written by the same author as the macros that would read it, so there is no credential to