diff --git a/CHANGES.md b/CHANGES.md index d4a8996..40b0878 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,17 @@ release promotes it to a version heading; the `bump-version` skill does that. ## Development +## 0.4.1 -- 2026-08-11 + +### Added + +- `admin.public` serves the whole admin API unauthenticated; off by default. + +### Fixed + +- `admin.groups` without `admin` left the write actions no legal value; it now + means public. + ## 0.4.0 -- 2026-08-11 ### Added diff --git a/Cargo.lock b/Cargo.lock index d278c41..1f7e1f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -591,7 +591,7 @@ dependencies = [ [[package]] name = "doppel-admin" -version = "0.4.0" +version = "0.4.1" dependencies = [ "async-trait", "axum", @@ -610,7 +610,7 @@ dependencies = [ [[package]] name = "doppel-cli" -version = "0.4.0" +version = "0.4.1" dependencies = [ "anyhow", "clap", @@ -631,7 +631,7 @@ dependencies = [ [[package]] name = "doppel-core" -version = "0.4.0" +version = "0.4.1" dependencies = [ "arc-swap", "async-trait", @@ -654,7 +654,7 @@ dependencies = [ [[package]] name = "doppel-proxy" -version = "0.4.0" +version = "0.4.1" dependencies = [ "axum", "doppel-core", @@ -672,7 +672,7 @@ dependencies = [ [[package]] name = "doppel-render" -version = "0.4.0" +version = "0.4.1" dependencies = [ "doppel-core", "minijinja", @@ -681,7 +681,7 @@ dependencies = [ [[package]] name = "doppel-store-postgres" -version = "0.4.0" +version = "0.4.1" dependencies = [ "async-trait", "doppel-core", @@ -696,7 +696,7 @@ dependencies = [ [[package]] name = "doppel-telemetry" -version = "0.4.0" +version = "0.4.1" dependencies = [ "doppel-core", "sentry", diff --git a/Cargo.toml b/Cargo.toml index e4d2c00..d06a5d5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["crates/*"] resolver = "3" [workspace.package] -version = "0.4.0" +version = "0.4.1" edition = "2024" rust-version = "1.94" license = "Apache-2.0" diff --git a/crates/doppel-admin/src/access.rs b/crates/doppel-admin/src/access.rs index f1581d4..5ceb96e 100644 --- a/crates/doppel-admin/src/access.rs +++ b/crates/doppel-admin/src/access.rs @@ -176,11 +176,22 @@ pub fn authorize( } } +/// So a public configuration can be answered with a borrow like any other. +const PUBLIC: Subjects = Subjects::Public; + fn effective_subjects<'a>( admin: &'a AdminConfig, proxy: Option<&'a ProxyConfig>, action: Action, ) -> &'a Subjects { + // `public: true`, or `groups: []`, makes every action public -- including a + // proxy's overrides, which is why this comes before them. A per-proxy + // `read: admin` under a public configuration would otherwise be the one + // thing still asking for a token, which is not what "public" was set for. + if admin.is_public() { + return &PUBLIC; + } + if action.overridable() && let Some(overrides) = proxy.and_then(|p| p.access.as_ref()) { @@ -417,6 +428,64 @@ proxies: ); } + /// `public: true` answers every action publicly, and the fixture's `access` + /// is deliberately restrictive so this cannot pass by the defaults being + /// permissive already. Enumerated over every action rather than spot-checked: + /// the point of the flag is that nothing is left needing a token. + #[test] + fn a_public_admin_api_authorises_every_action_for_anyone() { + let config = load_from_str(&CONFIG.replacen(" access:", " public: true\n access:", 1)) + .expect("fixture must parse"); + for action in [ + Action::List, + Action::Read, + Action::Create, + Action::Update, + Action::Delete, + Action::Upload, + ] { + assert!( + authorize(&config.admin, None, action, &Caller::Anonymous).is_ok(), + "`{}` must be public", + action.as_str() + ); + } + } + + /// `groups: []` names nobody, so it means the same thing. Worth its own test + /// because it is the spelling an operator reaches by trying to lock the + /// configuration down, which is the opposite of what it does. + #[test] + fn an_empty_groups_list_authorises_every_action_too() { + let config = load_from_str(&CONFIG.replacen(" access:", " groups: []\n access:", 1)) + .expect("fixture must parse"); + assert!(config.admin.is_public()); + assert!(authorize(&config.admin, None, Action::Delete, &Caller::Anonymous).is_ok()); + } + + /// A per-proxy override cannot claw back a token requirement under a public + /// configuration -- it would be the one thing still asking for a token, which + /// is not what the flag was set for. + #[test] + fn a_proxy_override_does_not_survive_a_public_admin_api() { + let config = load_from_str(&CONFIG.replacen(" access:", " public: true\n access:", 1)) + .expect("fixture must parse"); + let locked = config + .proxies + .iter() + .find(|proxy| proxy.access.is_some()) + .expect("the fixture must define a proxy with overrides"); + assert!( + authorize( + &config.admin, + Some(locked), + Action::Read, + &Caller::Anonymous + ) + .is_ok() + ); + } + #[test] fn the_configured_header_name_is_the_one_read() { let c = config(); diff --git a/crates/doppel-core/src/config/admin.rs b/crates/doppel-core/src/config/admin.rs index eec143c..3b75940 100644 --- a/crates/doppel-core/src/config/admin.rs +++ b/crates/doppel-core/src/config/admin.rs @@ -121,18 +121,40 @@ pub struct AdminConfig { /// each unique. `DOPPEL_ADMIN_TOKENS` can supply these instead. #[serde(default)] pub tokens: Vec, + /// Serve the whole admin API unauthenticated. + /// + /// `false` by default. `true` makes every action `public` and leaves no name + /// to reference, so `groups` is effectively empty and `access` effectively + /// all-public -- whatever either of them says. Anything they did say is + /// reported as a startup advisory rather than refused, so a configuration + /// being made temporarily public does not have to be gutted first and + /// rebuilt afterwards. + /// + /// This overrides rule V34, which otherwise refuses a public write action. + /// V34 exists so that an unauthenticated writable proxy set cannot happen by + /// omission; a field named `public` set to `true` is not an omission. Think + /// of it as the flag V34 was holding the line for. + /// + /// An `Option` and skipped when absent, for the reason given on `groups`: + /// adding it must not change the canonical YAML of configurations written + /// before it existed. Read it through [`AdminConfig::is_public`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub public: Option, /// Which names `access` may reference, here and in a proxy's overrides. /// /// `["*"]`, the default, allows any. A concrete list allows exactly those: /// `["admin", "ci"]` permits `admin` and `ci` and refuses `user`. /// - /// `[]` allows none, and that is stronger than it looks: every action - /// defaults to the `admin` group, so an empty list makes the defaults - /// themselves violations and every one of the six actions has to be written - /// out as `public`. + /// `[]` names nobody, which leaves no action anything to reference -- so it + /// means the same thing as `public: true`, and is treated as it. It used to + /// mean a configuration with no legal value for `create`, `update`, `delete` + /// or `upload`: V36 refused their `admin` default and V34 refused `public`, + /// so the document could not be written at all. /// - /// `public` is never governed by this. It is the absence of a subject rather - /// than a name, so an allow-list has nothing to say about it. + /// `public` and `admin` are never governed by this. `public` is the absence + /// of a subject rather than a name; `admin` is the fallback every action + /// already has, and a list that revoked it would produce the same + /// unsatisfiable state as `[]` once did. /// /// Checked by rule V36, not by this type: it compares one field against /// another, which is what is left for the rule set once the types have taken @@ -159,12 +181,41 @@ impl AdminConfig { /// default. /// /// One place resolves it, so no caller has to remember that absent and - /// `["*"]` mean the same thing while `[]` means the opposite -- which is - /// exactly the confusion an `Option>` invites. + /// `["*"]` mean the same thing while `[]` means something else entirely -- + /// which is exactly the confusion an `Option>` invites. #[must_use] pub fn allowed_groups(&self) -> &[AllowedGroup] { self.groups.as_deref().unwrap_or(&ANY_GROUP) } + + /// Whether the admin API is served unauthenticated. + /// + /// True for `public: true`, and equally for `groups: []`: a list naming + /// nobody leaves every action nothing to reference, so all-public is the only + /// reading of it that describes a configuration that can run. + /// + /// Everything that consults `access` goes through here, so "public" cannot + /// mean one thing to the rule set and another to the code that authorises a + /// request -- which is the failure this being a derived value rather than a + /// rewritten document is meant to prevent. + #[must_use] + pub fn is_public(&self) -> bool { + self.public.unwrap_or(false) || self.groups.as_deref().is_some_and(<[_]>::is_empty) + } + + /// What `access` amounts to once `is_public` is taken into account. + /// + /// `None` when the configuration is public, which every caller reads as + /// "`Subjects::Public` for every action". Returning the borrowed `access` in + /// the ordinary case keeps the common path allocation-free. + #[must_use] + pub fn effective_access(&self) -> Option<&AccessConfig> { + if self.is_public() { + None + } else { + Some(&self.access) + } + } } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, utoipa::ToSchema)] diff --git a/crates/doppel-core/src/validate/access.rs b/crates/doppel-core/src/validate/access.rs index 27a5247..7929d8d 100644 --- a/crates/doppel-core/src/validate/access.rs +++ b/crates/doppel-core/src/validate/access.rs @@ -30,13 +30,22 @@ pub(super) fn check(config: &Config, v: &mut Violations) { // rewrite the proxy set. That is far more often a mistake than an intent, // and startup is the last cheap moment to catch it. Reads stay allowed to // be public -- `/status` and a proxy listing give nothing away. + // + // Skipped for a public configuration. V34's job is to stop an + // unauthenticated writable proxy set happening by *omission*; `public: true` + // -- or the `groups: []` that means the same -- is the operator saying it in + // as many words, and refusing it would leave the flag with no effect it could + // ever have. let access = &config.admin.access; for (action, subjects) in [ ("create", &access.create), ("update", &access.update), ("delete", &access.delete), ("upload", &access.upload), - ] { + ] + .into_iter() + .filter(|_| !config.admin.is_public()) + { if matches!(subjects, Subjects::Public) { v.push( format!("admin.access.{action}"), @@ -53,11 +62,19 @@ pub(super) fn check(config: &Config, v: &mut Violations) { // added later ends up covering the admin block and quietly forgetting the // per-proxy overrides. let known = known_subjects(config); + let allowed = config.admin.allowed_groups(); + // A public configuration references nothing: `access` is answered as + // `public` whatever it says. Checking it anyway would refuse the very names + // `public: true` exists to override -- and those are reported as a startup + // advisory instead, which is a remark rather than a refusal. + let public = config.admin.is_public(); for (path, subjects) in access_sites(config) { // V27 check_subjects(subjects, &known, &path, v); // V36 - check_allowed_groups(subjects, config.admin.allowed_groups(), &path, v); + if !public { + check_allowed_groups(subjects, allowed, &path, v); + } } } @@ -96,12 +113,37 @@ fn access_sites(config: &Config) -> Vec<(String, &Subjects)> { sites } +/// The group `admin.groups` cannot exclude. +/// +/// Every action defaults to `admin`, and V34 refuses `public` for the four write +/// actions -- so a list omitting `admin` leaves `create`, `update`, `delete` and +/// `upload` with no legal value at all. That is not a lockdown, it is a document +/// nobody can write: +/// +/// ```text +/// admin.access.create: `admin` is not an allowed group; ... +/// admin.access.create: `create` must not be public: ...Name a token or a group. +/// ``` +/// +/// So `admin` is exempt, exactly as `public` is. An allow-list bounds the names +/// an operator may *hand access to*; it cannot revoke the fallback every action +/// already has, because forbidding that produces no reachable state. +/// +/// This is about a *non-empty* list that happens to omit `admin` -- +/// `groups: []` no longer reaches here at all, since naming nobody is read as +/// `public: true` and V36 is skipped for a public configuration. +/// +/// `user`, the other predefined group, is *not* exempt: nothing defaults to it, +/// so refusing it forecloses nothing. +const ALWAYS_ALLOWED_GROUP: &str = "admin"; + /// V36: a name `access` references has to be one `admin.groups` allows. /// -/// `Subjects::Public` is never checked. `public` is the absence of a subject -/// rather than a name, so an allow-list has nothing to say about it -- and a -/// configuration reduced to `groups: []` still has to be able to express "anyone -/// may read this". +/// `Subjects::Public` is never checked: `public` is the absence of a subject +/// rather than a name, so an allow-list has nothing to say about it. `admin` is +/// exempt too; see [`ALWAYS_ALLOWED_GROUP`]. +/// +/// Not called at all for a public configuration -- see the caller. fn check_allowed_groups( subjects: &Subjects, allowed: &[crate::config::AllowedGroup], @@ -112,27 +154,35 @@ fn check_allowed_groups( return; }; for name in names { - if allowed.iter().any(|entry| entry.permits(name.as_str())) { + if name == ALWAYS_ALLOWED_GROUP || allowed.iter().any(|entry| entry.permits(name.as_str())) + { continue; } - // The message names the list rather than only the rejection: the reader - // has to decide whether to change the reference or widen the list, and - // cannot do either without seeing what is currently permitted. - let permitted = if allowed.is_empty() { - "`admin.groups` is empty, so only `public` may be used".to_owned() - } else { - format!( - "`admin.groups` allows only {}", - allowed - .iter() - .map(|entry| format!("`{entry}`")) - .collect::>() - .join(", ") - ) - }; + // The message names everything permitted rather than only the + // rejection: the reader has to decide whether to change the reference or + // widen the list, and cannot do either without seeing what is currently + // allowed. + // + // Assembled as a set so `admin` appears once whether or not the list + // names it. Spelling the exemptions as a suffix printed + // "`admin`, `ci`, `admin` and `public`" for `groups: ["admin", "ci"]`, + // which reads like a bug in the very message meant to clarify things. + let mut permitted: Vec = allowed.iter().map(|entry| entry.to_string()).collect(); + for exempt in [ALWAYS_ALLOWED_GROUP, "public"] { + if !permitted.iter().any(|name| name == exempt) { + permitted.push(exempt.to_owned()); + } + } + permitted.sort_unstable(); + let permitted = permitted + .iter() + .map(|name| format!("`{name}`")) + .collect::>() + .join(", "); + v.push( path, - format!("`{name}` is not an allowed group: {permitted}"), + format!("`{name}` is not an allowed group; `admin.access` may name only {permitted}"), ); } } diff --git a/crates/doppel-core/src/validate/advisory.rs b/crates/doppel-core/src/validate/advisory.rs index 0a5a3d7..ae8999d 100644 --- a/crates/doppel-core/src/validate/advisory.rs +++ b/crates/doppel-core/src/validate/advisory.rs @@ -34,6 +34,61 @@ pub fn startup_advisories(config: &Config) -> Vec { } } + if config.admin.is_public() { + // Said first and unconditionally: an unauthenticated admin API is the + // single most consequential thing a configuration can turn on, and it is + // reachable two ways -- `public: true`, or the `groups: []` that means + // the same. Someone who wrote only the latter may not realise which they + // chose. + out.push( + if config.admin.public.unwrap_or(false) { + "admin.public is true: the whole admin API is served \ + unauthenticated, including the actions that rewrite the proxy \ + set" + } else { + "admin.groups is an empty list, which names nobody and therefore \ + means the same as `admin.public: true`: the whole admin API is \ + served unauthenticated. Set `public: true` if that was the \ + intent, or name the groups you meant to allow" + } + .to_owned(), + ); + + // Anything the document still says about who may do what is dead, and + // saying so is the difference between an operator seeing an override and + // an operator believing a token still guards something. + // + // Every test here is against what the *document* holds, not against the + // resolved value. `allowed_groups()` returns the `["*"]` default for an + // absent `groups`, so asking it would have reported `admin.groups` as + // overridden under `public: true` in a configuration that never + // mentioned it -- naming a field the operator did not write as one of + // their settings being ignored. + let mut overridden: Vec<&str> = Vec::new(); + if config.admin.access != crate::config::AccessConfig::default() { + overridden.push("admin.access"); + } + if config + .admin + .groups + .as_deref() + .is_some_and(|groups| !groups.is_empty()) + { + overridden.push("admin.groups"); + } + if config.proxies.iter().any(|proxy| proxy.access.is_some()) { + overridden.push("a proxy's access overrides"); + } + if !overridden.is_empty() { + out.push(format!( + "{} {} ignored while the admin API is public; every action \ + answers as `public` regardless", + overridden.join(", "), + if overridden.len() == 1 { "is" } else { "are" } + )); + } + } + for proxy in &config.proxies { if proxy.url.has_credentials() { out.push(format!( @@ -112,6 +167,74 @@ proxies: assert!(!notes[0].contains("secret"), "{}", notes[0]); } + /// An unauthenticated admin API is the most consequential thing a + /// configuration can turn on, so it is said out loud even when nothing is + /// being overridden. + #[test] + fn declaring_the_admin_api_public_is_reported() { + let text = raw(8080, 8081).replacen(" tokens:", " public: true\n tokens:", 1); + let notes = startup_advisories(&load_from_str(&text).unwrap()); + assert!( + notes + .iter() + .any(|note| note.contains("admin.public is true")), + "{notes:?}" + ); + // This fixture writes no `access` and no `groups`, so nothing is being + // overridden and nothing may claim to be. Asking `allowed_groups()` + // rather than the field reported `admin.groups` here, naming a setting + // the operator never wrote. + assert!( + !notes.iter().any(|note| note.contains("ignored while")), + "nothing was overridden: {notes:?}" + ); + } + + /// The spelling an operator reaches by trying to *restrict* things says the + /// same thing, so the advisory names which of the two they wrote and how to + /// say the other. + #[test] + fn an_empty_groups_list_is_reported_as_meaning_public() { + let text = raw(8080, 8081).replacen(" tokens:", " groups: []\n tokens:", 1); + let notes = startup_advisories(&load_from_str(&text).unwrap()); + let note = notes + .iter() + .find(|note| note.contains("admin.groups is an empty list")) + .unwrap_or_else(|| panic!("{notes:?}")); + assert!(note.contains("public: true"), "{note}"); + } + + /// What the document still says about who may do what is dead under a public + /// API, and the difference between saying so and not is an operator who + /// believes a token still guards something. + #[test] + fn access_and_groups_overridden_by_a_public_api_are_named() { + let text = raw(8080, 8081) + .replacen( + " tokens:", + " public: true\n groups: [\"ci\"]\n tokens:", + 1, + ) + .replace(" access: {}", " access:\n read: public"); + let notes = startup_advisories(&load_from_str(&text).unwrap()); + let note = notes + .iter() + .find(|note| note.contains("ignored while the admin API is public")) + .unwrap_or_else(|| panic!("{notes:?}")); + assert!(note.contains("admin.access"), "{note}"); + assert!(note.contains("admin.groups"), "{note}"); + } + + /// And a configuration that is not public says none of it. + #[test] + fn a_private_admin_api_produces_no_public_advisory() { + let notes = startup_advisories(&config(8080, 8081)); + assert!( + !notes.iter().any(|note| note.contains("public")), + "{notes:?}" + ); + } + #[test] fn a_privileged_port_is_still_a_legal_configuration() { // The point of an advisory rather than a rule: running on port 80 diff --git a/crates/doppel-core/src/validate/mod.rs b/crates/doppel-core/src/validate/mod.rs index 91627a8..677b3ca 100644 --- a/crates/doppel-core/src/validate/mod.rs +++ b/crates/doppel-core/src/validate/mod.rs @@ -210,41 +210,61 @@ proxies: let text = good() .replace(" access:", " groups: [\"admin\", \"ci\"]\n access:") .replace("read: public", "read: user"); - assert_violation(&text, "admin.access.read", "allows only `admin`, `ci`"); + assert_violation( + &text, + "admin.access.read", + "may name only `admin`, `ci`, `public`", + ); + } + + /// `groups: []` names nobody, so nothing can be granted to anyone and the + /// only reading that describes a runnable configuration is all-public. + /// + /// It used to be unsatisfiable: V36 refused the `admin` every action + /// defaults to, V34 refused `public` for the four writes, and no value + /// existed that both would accept. The regression is worth naming because + /// nothing failed -- the configuration was simply impossible to write. + #[test] + fn v36_an_empty_list_means_public_rather_than_an_impossible_document() { + let text = good().replace(" access:", " groups: []\n access:"); + let config = load_from_str(&text).unwrap(); + assert_eq!(validate(&config), Ok(())); + assert!(config.admin.is_public()); } - /// `[]` is the deliberate lockdown: no name may be referenced at all, and - /// the message says so rather than listing nothing. + /// The same for the flag that says it in as many words. V34 does not apply: + /// its job is to stop an unauthenticated writable proxy set happening by + /// omission, and this is the opposite of an omission. #[test] - fn v36_an_empty_list_permits_no_name_and_says_why() { + fn v34_does_not_refuse_writes_when_the_admin_api_is_declared_public() { let text = good() - .replace(" access:", " groups: []\n access:") - .replace("update: user1", "update: admin"); + .replace(" access:", " public: true\n access:") + .replace("update: user1", "update: public\n create: public"); + let config = load_from_str(&text).unwrap(); + assert_eq!(validate(&config), Ok(())); + assert!(config.admin.is_public()); + } + + /// And `public` still refuses a write when nothing declared the API public, + /// which is the case V34 was written for. + #[test] + fn v34_still_refuses_a_public_write_by_omission() { assert_violation( - &text, + &good().replace("update: user1", "update: public"), "admin.access.update", - "`admin.groups` is empty, so only `public` may be used", + "must not be public", ); } - /// `public` is the absence of a subject rather than a name, so an - /// allow-list has nothing to say about it -- and a configuration locked down - /// to `groups: []` still has to be able to express "anyone may read this". + /// A list that omits `admin` would otherwise recreate the impossible + /// document: every action defaults to `admin`, so refusing it leaves the + /// writes with no legal value again. #[test] - fn v36_public_is_never_governed_by_the_list() { + fn v36_never_refuses_admin_even_when_the_list_omits_it() { let text = good() - .replace(" access:", " groups: []\n access:") - .replace("update: user1", "update: public\n create: public"); - let config = load_from_str(&text).unwrap(); - // V34 refuses a public *write*, which is a different rule and still - // applies; `read: public` is what is being asserted here. - let violations = validate(&config).unwrap_err(); - assert!( - violations - .iter() - .all(|violation| violation.path != "admin.access.read"), - "`read: public` must not be a V36 violation: {violations:?}" - ); + .replace(" access:", " groups: [\"ci\"]\n access:") + .replace("update: user1", "update: admin"); + assert_eq!(validate(&load_from_str(&text).unwrap()), Ok(())); } /// A proxy's overrides are checked too. V27 and V36 walk one shared list of diff --git a/crates/doppel-store-postgres/migrations/0004_admin_public.sql b/crates/doppel-store-postgres/migrations/0004_admin_public.sql new file mode 100644 index 0000000..4f36c71 --- /dev/null +++ b/crates/doppel-store-postgres/migrations/0004_admin_public.sql @@ -0,0 +1,8 @@ +-- `AdminConfig::public`: serve the admin API unauthenticated. +-- +-- Nullable, with no DDL default, for the reason given in 0003: the field is an +-- `Option` whose absence is a different document from `false` written out, +-- and the revision is derived from the document's canonical YAML. A column that +-- materialised `false` would make every configuration stored before this +-- migration fail its own revision check on the first load after it. +ALTER TABLE configurations ADD COLUMN admin_public BOOLEAN; diff --git a/crates/doppel-store-postgres/src/load.rs b/crates/doppel-store-postgres/src/load.rs index 7d425b1..c547a83 100644 --- a/crates/doppel-store-postgres/src/load.rs +++ b/crates/doppel-store-postgres/src/load.rs @@ -114,6 +114,7 @@ impl PostgresStore { }, tokens, access: json_column::(row, "admin_access")?, + public: row.try_get("admin_public").map_err(query_failed)?, groups: optional_json::>(row, "admin_groups")?, upload: UploadConfig { limit: byte_size(row, "admin_upload_limit")?, diff --git a/crates/doppel-store-postgres/src/save.rs b/crates/doppel-store-postgres/src/save.rs index 75390ed..27604c4 100644 --- a/crates/doppel-store-postgres/src/save.rs +++ b/crates/doppel-store-postgres/src/save.rs @@ -269,20 +269,22 @@ const UPDATE_HEADER: &str = "UPDATE configurations SET revision = $1, \ log_format = $8, control_socket = $9, templates_dir = $10, sentry_dsn = $11, \ admin_host = $12, admin_port = $13, admin_auth_header = $14, \ admin_upload_limit = $15, admin_access = $16, admin_groups = $17, \ - updated_at = now() \ + admin_public = $18, updated_at = now() \ WHERE name = $2 AND revision = $3"; const UPSERT_HEADER: &str = "INSERT INTO configurations \ (name, revision, admin_enable, server_host, server_port, log_level, log_format, \ control_socket, templates_dir, sentry_dsn, admin_host, admin_port, \ - admin_auth_header, admin_upload_limit, admin_access, admin_groups) \ - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16) \ + admin_auth_header, admin_upload_limit, admin_access, admin_groups, \ + admin_public) \ + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, \ + $17) \ ON CONFLICT (name) DO UPDATE SET revision = $2, \ admin_enable = $3, server_host = $4, server_port = $5, log_level = $6, \ log_format = $7, control_socket = $8, templates_dir = $9, sentry_dsn = $10, \ admin_host = $11, admin_port = $12, admin_auth_header = $13, \ admin_upload_limit = $14, admin_access = $15, admin_groups = $16, \ - updated_at = now()"; + admin_public = $17, updated_at = now()"; /// Bind the header values in `HEADER_COLUMNS` order. /// @@ -319,6 +321,7 @@ impl<'q> BindHeader<'q> for sqlx::query::Query<'q, Postgres, sqlx::postgres::PgA .as_ref() .map(|groups| serde_json::to_value(groups).unwrap_or(serde_json::Value::Null)), ) + .bind(config.admin.public) } } @@ -390,6 +393,7 @@ mod tests { "admin_upload_limit", "admin_access", "admin_groups", + "admin_public", ]; /// Every `column = $n` assignment in a statement. diff --git a/docs/usage/configuration.md b/docs/usage/configuration.md index 4330142..d894bcf 100644 --- a/docs/usage/configuration.md +++ b/docs/usage/configuration.md @@ -117,6 +117,7 @@ admin: enable: true host: "0.0.0.0" port: 8081 + public: false groups: ["*"] auth: header: X-Proxy-Authorization @@ -159,69 +160,70 @@ action out entirely, which is far more often a typo than an intent. `access` maps each action to `public`, a single name, or a list of names. An empty list means public. Names are token names or group names. +### `public`: serve the admin API unauthenticated + +```yaml +admin: + public: true +``` + +Every action answers as `public`, for anyone, with no token. `false` by default. + +This overrides rule V34, which otherwise refuses a public write action. V34 is +there so an unauthenticated writable proxy set cannot happen by *omission*; a +field called `public` set to `true` is not an omission. + +Anything `access`, `groups` or a proxy's overrides still say is ignored, and +startup says so rather than refusing the document -- so a configuration can be +made public temporarily without being gutted first: + +``` +admin.public is true: the whole admin API is served unauthenticated, including +the actions that rewrite the proxy set +admin.access, admin.groups are ignored while the admin API is public; every +action answers as `public` regardless +``` + ### `groups`: which names `access` may reference ```yaml admin: - groups: ["*"] # the default: any name may be referenced + groups: ["*"] # the default ``` -`groups` bounds the vocabulary `access` may draw on -- both here and in a -proxy's `access` overrides. It is checked by rule **V36**. +Bounds the vocabulary `access` may draw on, here and in a proxy's overrides. +Rule **V36**. -| `groups` | What `access` may reference | +| `groups` | `access` may name | |---|---| -| absent | any name. This is the default | -| `["*"]` | any name. The same thing, written down | -| `["admin", "ci"]` | `admin` and `ci`, and nothing else. `user` is refused | -| `[]` | no name at all: every one of the six actions has to be spelled `public` | - -`public` is **never** governed by `groups`. It is the absence of a subject rather -than a name, so an allow-list has nothing to say about it -- and a deployment -locked down to `groups: []` still has to be able to say "anyone may read this". - -!!! warning "`groups: []` is not a small change" - Every action defaults to the `admin` group, and `groups: []` forbids naming - `admin`. So the defaults themselves become violations, and a configuration - with `groups: []` is only valid if **all six** actions are written out as - `public`: - - ``` - admin.access.list: `admin` is not an allowed group: `admin.groups` is empty, so only `public` may be used - admin.access.create: `admin` is not an allowed group: ... - admin.access.update: `admin` is not an allowed group: ... - ``` - - That is what an empty allow-list means, taken literally, and it is reported - per action rather than once, so nothing is missed on the way to fixing it. - If the intent was "no *custom* groups", name the ones you do want -- - `["admin"]` -- rather than emptying the list. - -The default is permissive, which is the opposite of how `access` itself -defaults, on purpose. `access` defaults to `admin` because the cost of getting it -wrong is unauthenticated writes. `groups` defaults to `*` because the cost of -getting it wrong is an operator unable to name their own groups, and an -allow-list nobody asked for only ever surprises. - -The violation names what is permitted, not just what was refused: +| absent, or `["*"]` | anything. The default | +| `["admin", "ci"]` | `admin`, `ci`, and nothing else -- `user` is refused | +| `[]` | nobody, which is the same as `public: true`. See below | + +`public` and `admin` are always available whatever the list says. `public` is the +absence of a subject rather than a name; `admin` is the fallback every action +already has, and a list that revoked it would leave the four write actions with +no legal value at all. + +`groups: []` names nobody, so nothing can be granted to anyone -- which leaves +all-public as the only reading of it that describes a configuration that runs. It +is treated as `public: true`, and startup says which of the two you wrote. + +The violation names everything permitted, because the reader has to choose +between changing the reference and widening the list: ``` -admin.access.read: `user` is not an allowed group: `admin.groups` allows only `admin`, `ci` -proxies[0].access.update: `admin` is not an allowed group: `admin.groups` is empty, so only `public` may be used +admin.access.read: `user` is not an allowed group; `admin.access` may name only `admin`, `ci`, `public` ``` -That matters because the reader has to choose between changing the reference and -widening the list, and cannot do either without seeing the list. - Two things `groups` does not do. It does not create groups -- a name still has to -be a predefined group or one carried by a token, which is rule V27, and the two -rules are reported separately because widening `groups` and adding a token are -different fixes. And it is not authorisation: it constrains what a -*configuration* may say, so a caller's rights come from `access` as before. - -Since it applies to proxy overrides too, `POST` and `PUT /api/v1/proxies` refuse -a document whose `access` names something outside the list, with `400` and -`CONFIG_INVALID`. +be predefined or carried by a token, which is rule V27, reported separately +because widening the list and adding a token are different fixes. And it is not +authorisation: it bounds what a *configuration* may say, and a caller's rights +still come from `access`. + +`POST` and `PUT /api/v1/proxies` refuse a document whose `access` names something +outside the list, with `400` and `CONFIG_INVALID`. Every action defaults to the `admin` group, reads included. The most common configuration is the one nobody wrote, so the default has to be the safe one. diff --git a/doppel-config.schema.json b/doppel-config.schema.json index d22af5e..be21fd1 100644 --- a/doppel-config.schema.json +++ b/doppel-config.schema.json @@ -46,7 +46,7 @@ "type": "boolean" }, "groups": { - "description": "Which names `access` may reference, here and in a proxy's overrides.\n\n`[\"*\"]`, the default, allows any. A concrete list allows exactly those:\n`[\"admin\", \"ci\"]` permits `admin` and `ci` and refuses `user`.\n\n`[]` allows none, and that is stronger than it looks: every action\ndefaults to the `admin` group, so an empty list makes the defaults\nthemselves violations and every one of the six actions has to be written\nout as `public`.\n\n`public` is never governed by this. It is the absence of a subject rather\nthan a name, so an allow-list has nothing to say about it.\n\nChecked by rule V36, not by this type: it compares one field against\nanother, which is what is left for the rule set once the types have taken\neverything they can decide alone.\n\nAn `Option` rather than a `Vec` defaulting to `[\"*\"]`, and skipped when\nabsent, so that adding this field did not change the canonical YAML of\nevery configuration written before it existed. The revision is derived\nfrom that YAML, so a materialised default would have made every stored\nconfiguration fail its own revision check on the first load after the\nupgrade. Read it through [`AdminConfig::allowed_groups`].", + "description": "Which names `access` may reference, here and in a proxy's overrides.\n\n`[\"*\"]`, the default, allows any. A concrete list allows exactly those:\n`[\"admin\", \"ci\"]` permits `admin` and `ci` and refuses `user`.\n\n`[]` names nobody, which leaves no action anything to reference -- so it\nmeans the same thing as `public: true`, and is treated as it. It used to\nmean a configuration with no legal value for `create`, `update`, `delete`\nor `upload`: V36 refused their `admin` default and V34 refused `public`,\nso the document could not be written at all.\n\n`public` and `admin` are never governed by this. `public` is the absence\nof a subject rather than a name; `admin` is the fallback every action\nalready has, and a list that revoked it would produce the same\nunsatisfiable state as `[]` once did.\n\nChecked by rule V36, not by this type: it compares one field against\nanother, which is what is left for the rule set once the types have taken\neverything they can decide alone.\n\nAn `Option` rather than a `Vec` defaulting to `[\"*\"]`, and skipped when\nabsent, so that adding this field did not change the canonical YAML of\nevery configuration written before it existed. The revision is derived\nfrom that YAML, so a materialised default would have made every stored\nconfiguration fail its own revision check on the first load after the\nupgrade. Read it through [`AdminConfig::allowed_groups`].", "items": { "$ref": "#/$defs/AllowedGroup" }, @@ -66,6 +66,13 @@ "$ref": "#/$defs/Port", "description": "The TCP port the admin API listens on. Must differ from\n`server.port`." }, + "public": { + "description": "Serve the whole admin API unauthenticated.\n\n`false` by default. `true` makes every action `public` and leaves no name\nto reference, so `groups` is effectively empty and `access` effectively\nall-public -- whatever either of them says. Anything they did say is\nreported as a startup advisory rather than refused, so a configuration\nbeing made temporarily public does not have to be gutted first and\nrebuilt afterwards.\n\nThis overrides rule V34, which otherwise refuses a public write action.\nV34 exists so that an unauthenticated writable proxy set cannot happen by\nomission; a field named `public` set to `true` is not an omission. Think\nof it as the flag V34 was holding the line for.\n\nAn `Option` and skipped when absent, for the reason given on `groups`:\nadding it must not change the canonical YAML of configurations written\nbefore it existed. Read it through [`AdminConfig::is_public`].", + "type": [ + "boolean", + "null" + ] + }, "tokens": { "description": "The tokens that may call the admin API. Names and token values are\neach unique. `DOPPEL_ADMIN_TOKENS` can supply these instead.", "items": { diff --git a/main.example.yaml b/main.example.yaml index 18da04d..0bb5d79 100644 --- a/main.example.yaml +++ b/main.example.yaml @@ -31,11 +31,14 @@ admin: enable: true host: "0.0.0.0" port: 8081 + # Serve the admin API unauthenticated: every action answers as `public`, + # for anyone. Overrides `access`, `groups` and any proxy override, which + # startup then reports as ignored. Off by default. + public: false # Which names `access` may reference, here and per proxy. "*" is any; # ["admin", "ci"] allows exactly those. Never governs `public` itself. - # [] allows no name at all -- and since every action defaults to `admin`, - # that makes the defaults violations too, so all six would have to be - # written out as `public`. Rule V36. + # `public` and `admin` are always available whatever the list says. + # [] names nobody, which means the same as `public: true`. Rule V36. groups: ["*"] auth: header: X-Proxy-Authorization # expects "Bearer {token}"