From 64dafd1ada3eedd06ede44d273169ddaeaab8f40 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Mon, 14 Sep 2026 11:11:50 -0400 Subject: [PATCH] refactor(claim)!: run the shared onboarding steps from --check `didbot-claim --check` now runs `didbot-onboarding`'s steps rather than its own copy of the four checks, so the command and the policy page answer from one list. The report gains the zone delegation the deployment's whole chain of trust rests on, and each check says what it found rather than only that it held; a check waiting on an earlier step reads as blocked, not failed. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: Ib8e1ab90b3c67fd9c0c927452f204e658e38bd0d --- Cargo.lock | 1 + crates/didbot-claim/Cargo.toml | 1 + crates/didbot-claim/src/bin/didbot-claim.rs | 20 +- crates/didbot-claim/src/preflight.rs | 586 ++------------------ crates/didbot-fsm/src/lib.rs | 16 +- crates/didbot-onboarding/tests/steps.rs | 49 +- crates/didbot-pds/src/server_state.rs | 4 +- crates/didbot-reconcile/src/lib.rs | 6 +- docs/server-lifecycle.md | 7 +- 9 files changed, 114 insertions(+), 576 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7ec51c61..8a13b7d6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1144,6 +1144,7 @@ dependencies = [ "didbot-identity", "didbot-key", "didbot-lexicon", + "didbot-onboarding", "didbot-pds", "didbot-serve", "jacquard-common", diff --git a/crates/didbot-claim/Cargo.toml b/crates/didbot-claim/Cargo.toml index 874a1a62..48911d3f 100644 --- a/crates/didbot-claim/Cargo.toml +++ b/crates/didbot-claim/Cargo.toml @@ -15,6 +15,7 @@ path = "src/bin/didbot-claim.rs" [dependencies] didbot-http.workspace = true didbot-claim-check.workspace = true +didbot-onboarding.workspace = true didbot-identity.workspace = true didbot-key.workspace = true didbot-lexicon.workspace = true diff --git a/crates/didbot-claim/src/bin/didbot-claim.rs b/crates/didbot-claim/src/bin/didbot-claim.rs index a04c2b43..6ed353ed 100644 --- a/crates/didbot-claim/src/bin/didbot-claim.rs +++ b/crates/didbot-claim/src/bin/didbot-claim.rs @@ -61,9 +61,10 @@ use std::time::Duration; use didbot_claim::describe::HttpServerDescriber; use didbot_claim::identify::HttpDocumentFetcher; use didbot_claim::orchestrate::{claim, ClaimError}; -use didbot_claim::preflight::{preflight, SystemResolver}; +use didbot_claim::preflight::preflight; use didbot_claim::session_store::FileAuthStore; use didbot_claim::tls::ReqwestTlsProbe; +use didbot_onboarding::NativeEnvironment; use jacquard_common::session::SessionKey; use jacquard_oauth::atproto::AtprotoClientMetadata; use jacquard_oauth::client::OAuthClient; @@ -181,19 +182,12 @@ fn parse_args(args: &[String]) -> Result, String> { /// `--check`: the read-only half, alone. /// -/// Deliberately shares [`didbot_claim::preflight::preflight`] with nothing — -/// it *is* the thing a real claim runs first, so an operator who sees this -/// pass and then watches a claim fail on the same check has found a genuine -/// race rather than two implementations disagreeing. +/// The checks are `didbot-onboarding`'s, which is the same list the policy +/// page runs — so an operator who sees this pass and then watches a claim +/// fail on the same check has found a genuine race rather than two +/// implementations disagreeing. async fn check(hostname: &str) -> Result<(), String> { - let http = didbot_http::client(); - let report = preflight( - &SystemResolver, - &HttpDocumentFetcher::new(http.clone()), - &HttpServerDescriber::new(http), - hostname, - ) - .await; + let report = preflight(&NativeEnvironment::new(), hostname).await; println!("{hostname}"); print!("{report}"); diff --git a/crates/didbot-claim/src/preflight.rs b/crates/didbot-claim/src/preflight.rs index 510e4cf7..78d02c17 100644 --- a/crates/didbot-claim/src/preflight.rs +++ b/crates/didbot-claim/src/preflight.rs @@ -1,9 +1,12 @@ //! Whether a server can be claimed at all, checked from the outside. //! -//! Everything here answers one question — *can a stranger reach this -//! server, and is it the server it says it is* — and answers it from the -//! operator's own machine, which is the only place the answer means -//! anything. +//! Everything here answers one question — *can a stranger reach this server, +//! and is it the server it says it is* — and answers it from the operator's +//! own machine, which is the only place the answer means anything. +//! +//! The checks themselves are [`didbot_onboarding`]'s, and so is the order +//! they run in. This module is the projection a claim needs: the steps a +//! claim waits on, and the report `--check` prints. //! //! # Why this is not on the server //! @@ -30,193 +33,69 @@ //! //! # What this does not do //! -//! It does not *fix* anything. Keeping a zone this deployment manages -//! correct — noticing a missing record and republishing it — is a standing -//! obligation the server owns in every state, forever, and a separate state -//! machine; `plan/onboarding.md` carries the write-up. This module only ever -//! reports. -//! -//! # Two-valued, deliberately -//! -//! A check here is `Passed` or `Failed`, never "not looked at yet". Every -//! one of them runs to completion before this function returns, so a third -//! value would describe a state that cannot exist. That is the difference -//! between a synchronous command and a background probe: `didbot_fsm::Gates` -//! is three-valued because `didbot_reconcile` acts across ticks and needs -//! `Pending`, and nothing here does. +//! It reports. Keeping a zone this deployment manages correct — noticing a +//! missing record and republishing it — is a standing obligation the server +//! owns in every state, forever, and a separate state machine; +//! `plan/onboarding.md` carries the write-up. use std::fmt; -use std::net::IpAddr; -use std::time::Duration; -use crate::describe::ServerDescriber; -use crate::identify::DocumentFetcher; +use didbot_onboarding::{run_steps, Check, Environment, Run, Step, Target, Verdict}; -/// How long any one check waits before calling it a failure. +/// The steps a claim waits on, in the order they run. /// -/// A check is a question about reachability, so a probe that hangs *is* the -/// answer; a generous timeout only buys a longer wait for the same -/// conclusion. This is the per-check bound for `--check`, which is a -/// one-shot report — the claim flow proper waits much longer for TLS, on -/// purpose, because there it is waiting out an ACME order rather than -/// reporting a fact. See `didbot_claim::tls::wait_for_tls`. -pub const CHECK_TIMEOUT: Duration = Duration::from_secs(10); - -/// One thing that has to be true before a server can be claimed. -/// -/// Ordered as they are checked, which is also the order they fail in -/// practice: a name that does not resolve cannot present a certificate, and -/// a server that does not answer HTTPS cannot serve a document. -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -pub enum Check { - /// The hostname resolves, through an ordinary resolver. - /// - /// Checked from the operator's machine because that is where a claim is - /// written from and where every other consumer of this server will be - /// looking from. What it deliberately does *not* assert is that the - /// address belongs to any particular host: a server behind a load - /// balancer, a NAT or an anycast address correctly resolves to something - /// that is not itself, and the checks below reaching it is a stronger - /// test than any comparison would have been. - Dns, - /// TLS completes and something answers HTTPS on that name. - /// - /// Established by making a real request and caring only that a response - /// came back — any status. The chain and the name are verified against - /// the platform trust store as part of connecting, so a response *at - /// all* is the proof. - Tls, - /// `https:///.well-known/did.json` serves a DID document whose - /// `id` is `did:web:` and whose `#atproto` verification method - /// is a public key atproto verifies commits with. - /// - /// The document a claim will name, and the key it will bind - /// `subjectKey` to. If this fails, there is nothing for a - /// `bot.did.operator` record to point at. The key is checked here and - /// not only at write time so that `--check` and a real claim agree: - /// see [`crate::identify::signing_key`] for why an unreadable key is a - /// refusal rather than something to write and find out about later. - DidDocument, - /// `com.atproto.server.describeServer` agrees with that document. - /// - /// The check that catches two deployments answering one name, which is - /// the failure mode invisible from either of them. See - /// [`crate::describe`]. - ServerDescription, -} - -impl Check { - /// Every check, in the order [`preflight`] runs them. - pub const ALL: &'static [Check] = &[ - Check::Dns, - Check::Tls, - Check::DidDocument, - Check::ServerDescription, - ]; - - /// The kebab-case name used in output. - pub const fn as_str(self) -> &'static str { - match self { - Check::Dns => "dns", - Check::Tls => "tls", - Check::DidDocument => "did-document", - Check::ServerDescription => "server-description", - } - } -} - -impl fmt::Display for Check { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(self.as_str()) - } -} - -/// What one [`Check`] found. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum Outcome { - /// The outside world agrees. - Passed, - /// It does not, and this is what an operator reads to know which of the - /// check's failure modes this is. - Failed { - /// Why, in a sentence somebody reads once and acts on. - reason: String, - }, - /// An earlier check failed and this one was not attempted, because its - /// answer would have been noise rather than information — a certificate - /// cannot be presented for a name that does not resolve. - /// - /// Distinct from [`Self::Failed`] on purpose. Reporting "TLS failed" for - /// a hostname with no DNS sends an operator to look at certificates when - /// the problem is a delegation. - Skipped { - /// The check whose failure made this one moot. - after: Check, - }, -} - -impl Outcome { - /// Whether this check is satisfied. Only [`Self::Passed`] is. - pub fn passed(&self) -> bool { - matches!(self, Outcome::Passed) - } - - /// The reason, for a failure. - pub fn reason(&self) -> Option<&str> { - match self { - Outcome::Failed { reason } => Some(reason), - _ => None, - } - } -} - -impl fmt::Display for Outcome { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Outcome::Passed => f.write_str("ok"), - Outcome::Failed { reason } => write!(f, "FAILED: {reason}"), - Outcome::Skipped { after } => write!(f, "skipped ({after} failed first)"), - } - } -} - -/// Every check and what it found, in [`Check::ALL`] order. +/// The four a `bot.did.operator` record cannot be written without: the name +/// resolves, something answers on it, the document it serves is this +/// server's, and `describeServer` agrees. The rest of +/// [`didbot_onboarding::Step::ALL`] is about a server that has already been +/// claimed, so a command whose whole job is writing the claim does not wait +/// on it. +pub const CLAIM_STEPS: [Step; 4] = [Step::Zone, Step::Host, Step::Document, Step::Description]; + +/// Every check in [`CLAIM_STEPS`] and what it found. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Report { /// The hostname that was checked. pub hostname: String, - outcomes: Vec, + run: Run, } impl Report { - /// What one check found. - pub fn outcome(&self, check: Check) -> &Outcome { - let index = Check::ALL - .iter() - .position(|candidate| *candidate == check) - .expect("a Check appears in its own ALL"); - &self.outcomes[index] + /// What one check found, if it ran. + #[must_use] + pub fn outcome(&self, check: Check) -> Option<&Verdict> { + self.run.verdict(check) } /// Every check and its outcome, in order. - pub fn all(&self) -> impl Iterator { - Check::ALL.iter().copied().zip(self.outcomes.iter()) + pub fn all(&self) -> impl Iterator { + self.run + .checks() + .map(|outcome| (outcome.check, &outcome.verdict)) } /// Whether every check passed, which is the whole question this module /// answers. + #[must_use] pub fn claimable(&self) -> bool { - self.outcomes.iter().all(Outcome::passed) + self.run.passed() } /// The first check that failed, if any. /// /// The first rather than all of them, because the later ones are - /// [`Outcome::Skipped`] and a caller reporting "fix this" wants the + /// [`Verdict::Blocked`] and a caller reporting "fix this" wants the /// cause, not the consequences. + #[must_use] pub fn first_failure(&self) -> Option<(Check, &str)> { - self.all() - .find_map(|(check, outcome)| outcome.reason().map(|reason| (check, reason))) + self.run.first_failure() + } + + /// The run behind this report, for a caller that wants the steps rather + /// than the lines. + #[must_use] + pub fn run(&self) -> &Run { + &self.run } } @@ -230,379 +109,16 @@ impl fmt::Display for Report { } } -/// Resolves a hostname to addresses. The seam a test substitutes; -/// [`SystemResolver`] is the only production implementation. -// `async fn` in a public trait forgoes an auto `Send` bound on its future -- -// fine here for the same reason `crate::tls::TlsProbe` gives: every use is -// generic, never a trait object, and nothing here spawns the future onto -// another task. -#[allow(async_fn_in_trait)] -pub trait HostResolver { - /// Resolves `hostname`, or reports why it could not. - async fn resolve(&self, hostname: &str) -> Result, String>; -} - -/// Resolves through the operating system's own resolver. -/// -/// Deliberately not a DNS client of this crate's own. The answer that -/// matters is the one an ordinary consumer's stub resolver gets, and the -/// operator's machine is the closest available stand-in for one — vendoring -/// a resolver and a root hint list would answer a subtly different question -/// than "will this work for people." -#[derive(Debug, Default, Clone, Copy)] -pub struct SystemResolver; - -impl HostResolver for SystemResolver { - async fn resolve(&self, hostname: &str) -> Result, String> { - let target = format!("{hostname}:443"); - match tokio::time::timeout(CHECK_TIMEOUT, tokio::net::lookup_host(target)).await { - Err(_) => Err(format!( - "the resolver did not answer within {CHECK_TIMEOUT:?}" - )), - Ok(Err(error)) => Err(error.to_string()), - Ok(Ok(addrs)) => Ok(addrs.map(|addr| addr.ip()).collect()), - } - } -} - -/// Runs every check against `hostname` and reports what it found. -/// -/// Never writes anything and never authenticates: this is the read-only -/// half of `didbot-claim`, exposed as `didbot-claim --check ` and -/// run again as the first half of a real claim. An operator who runs it gets -/// the same answers the claim flow would get, without an OAuth round trip -/// and without a record. -/// -/// # Why a failure stops the run +/// Runs [`CLAIM_STEPS`] against `hostname` and reports what they found. /// -/// Later checks are [`Outcome::Skipped`] rather than attempted, because -/// their failures would be *consequences* and reporting four failures for -/// one cause is how an operator ends up debugging the wrong thing. The -/// dependency is real in every case: a certificate cannot be presented for a -/// name that does not resolve, and a document cannot be served over a -/// connection that does not complete. -pub async fn preflight(resolver: &R, fetcher: &F, describer: &D, hostname: &str) -> Report -where - R: HostResolver, - F: DocumentFetcher, - D: ServerDescriber, -{ - let mut outcomes = Vec::with_capacity(Check::ALL.len()); - let mut failed_at: Option = None; - - // DNS. - let dns = match resolver.resolve(hostname).await { - Ok(addrs) if addrs.is_empty() => Outcome::Failed { - reason: format!("{hostname} resolves to no addresses"), - }, - Ok(_) => Outcome::Passed, - Err(error) => Outcome::Failed { - reason: format!("{hostname} does not resolve: {error}"), - }, - }; - if !dns.passed() { - failed_at = Some(Check::Dns); - } - outcomes.push(dns); - - // TLS, and then the document, over the same fetcher: resolving the - // `did:web` document *is* an HTTPS request, so a fetch that succeeds - // proves both. They stay separate checks because they fail for different - // reasons and send an operator to different places. - let url = format!("https://{hostname}/.well-known/did.json"); - let fetched = match failed_at { - Some(after) => { - outcomes.push(Outcome::Skipped { after }); - outcomes.push(Outcome::Skipped { after }); - outcomes.push(Outcome::Skipped { after }); - return Report { - hostname: hostname.to_owned(), - outcomes, - }; - } - None => fetcher.fetch(&url).await, - }; - - let document = match &fetched { - Ok(body) => { - outcomes.push(Outcome::Passed); - match crate::identify::parse_document(hostname, &url, body) - .map_err(|error| error.to_string()) - .and_then(|document| { - crate::identify::signing_key(hostname, &document) - .map_err(|error| error.to_string())?; - Ok(document) - }) { - Ok(document) => { - outcomes.push(Outcome::Passed); - Some(document) - } - Err(reason) => { - outcomes.push(Outcome::Failed { reason }); - failed_at = Some(Check::DidDocument); - None - } - } - } - Err(error) => { - outcomes.push(Outcome::Failed { - reason: format!("no usable HTTPS connection to {hostname}: {error}"), - }); - outcomes.push(Outcome::Skipped { after: Check::Tls }); - failed_at = Some(Check::Tls); - None - } - }; - - // describeServer, cross-checked against the document. - let description = match (&document, failed_at) { - (Some(document), _) => { - match crate::describe::confirm(describer, hostname, &document.id).await { - Ok(()) => Outcome::Passed, - Err(error) => Outcome::Failed { - reason: error.to_string(), - }, - } - } - (None, Some(after)) => Outcome::Skipped { after }, - // Unreachable: `document` is `None` only when something above - // failed. Handled rather than panicking, because a report that - // reached this arm somehow is still more useful than a crash. - (None, None) => Outcome::Skipped { - after: Check::DidDocument, - }, - }; - outcomes.push(description); - +/// Never writes anything and never authenticates: this is the read-only half +/// of `didbot-claim`, exposed as `didbot-claim --check ` and run +/// again as the first half of a real claim. An operator who runs it gets the +/// same answers the claim flow would get, without an OAuth round trip and +/// without a record. +pub async fn preflight(env: &E, hostname: &str) -> Report { Report { hostname: hostname.to_owned(), - outcomes, - } -} - -#[cfg(test)] -mod tests { - use super::*; - use didbot_identity::resolve::ResolveError; - - struct Resolver(Result, String>); - impl HostResolver for Resolver { - async fn resolve(&self, _hostname: &str) -> Result, String> { - self.0.clone() - } - } - - struct Fetcher(Result); - impl DocumentFetcher for Fetcher { - async fn fetch(&self, _url: &str) -> Result { - self.0.clone() - } - } - - struct Describer(Result); - impl ServerDescriber for Describer { - async fn describe(&self, _hostname: &str) -> Result { - self.0.clone() - } - } - - fn addr() -> Vec { - vec!["203.0.113.7".parse().expect("a literal address")] - } - - /// A document that would really pass: the `id` the hostname implies and - /// an `#atproto` key a claim could bind `subjectKey` to. Both, because - /// `--check` reporting claimable for a document a real claim then - /// refuses is the one thing this module promises does not happen. - fn document(host: &str) -> String { - document_with_key( - host, - Some( - &didbot_key::SigningKey::generate() - .verifying_key() - .to_multibase(), - ), - ) - } - - fn document_with_key(host: &str, key: Option<&str>) -> String { - let did = format!("did:web:{host}"); - let mut value = serde_json::json!({ - "@context": ["https://www.w3.org/ns/did/v1"], - "id": did, - "verificationMethod": [], - }); - if let Some(key) = key { - value["verificationMethod"] = serde_json::json!([{ - "id": format!("{did}#atproto"), - "type": "Multikey", - "controller": did, - "publicKeyMultibase": key, - }]); - } - value.to_string() - } - - /// `--check` and a real claim read the same document the same way. A - /// document with no `#atproto` key, or one naming something that is not - /// an atproto public key, is not claimable -- and reporting it as - /// claimable would send an operator through an OAuth consent screen to - /// reach a refusal `--check` already had the answer to. - #[tokio::test] - async fn a_document_without_a_usable_signing_key_is_not_claimable() { - let host = "pds.example.test"; - for key in [None, Some("zNotARealKeyAtAll")] { - let report = preflight( - &Resolver(Ok(addr())), - &Fetcher(Ok(document_with_key(host, key))), - &Describer(Ok(format!("did:web:{host}"))), - host, - ) - .await; - assert!(!report.claimable(), "{key:?} was reported claimable"); - let (check, _) = report.first_failure().expect("a failure"); - assert_eq!(check, Check::DidDocument, "{key:?}"); - // The document is the cause, so the check below it is a - // consequence and must not be attempted. - assert_eq!( - report.outcome(Check::ServerDescription), - &Outcome::Skipped { - after: Check::DidDocument - }, - "{key:?}" - ); - } - } - - #[tokio::test] - async fn everything_passing_is_claimable() { - let host = "pds.example.test"; - let report = preflight( - &Resolver(Ok(addr())), - &Fetcher(Ok(document(host))), - &Describer(Ok(format!("did:web:{host}"))), - host, - ) - .await; - assert!(report.claimable(), "{report}"); - assert_eq!(report.first_failure(), None); - } - - /// The property the whole design turns on: one cause, one failure, and - /// the consequences reported as skipped rather than as three more - /// problems to debug. - #[tokio::test] - async fn a_dns_failure_skips_everything_downstream() { - let report = preflight( - &Resolver(Err("NXDOMAIN".into())), - &Fetcher(Ok(document("pds.example.test"))), - &Describer(Ok("did:web:pds.example.test".into())), - "pds.example.test", - ) - .await; - assert!(!report.claimable()); - let (check, reason) = report.first_failure().expect("a failure"); - assert_eq!(check, Check::Dns); - assert!(reason.contains("NXDOMAIN"), "{reason}"); - for later in [Check::Tls, Check::DidDocument, Check::ServerDescription] { - assert_eq!( - report.outcome(later), - &Outcome::Skipped { after: Check::Dns }, - "{later} should not have been attempted" - ); - } - } - - #[tokio::test] - async fn resolving_to_nothing_is_a_dns_failure() { - let report = preflight( - &Resolver(Ok(Vec::new())), - &Fetcher(Ok(document("pds.example.test"))), - &Describer(Ok("did:web:pds.example.test".into())), - "pds.example.test", - ) - .await; - assert_eq!( - report.first_failure().map(|(check, _)| check), - Some(Check::Dns) - ); - } - - #[tokio::test] - async fn an_unreachable_server_fails_tls_and_not_the_document() { - let report = preflight( - &Resolver(Ok(addr())), - &Fetcher(Err(ResolveError::Transport { - url: "https://pds.example.test/.well-known/did.json".into(), - message: "connection refused".into(), - })), - &Describer(Ok("did:web:pds.example.test".into())), - "pds.example.test", - ) - .await; - assert_eq!( - report.first_failure().map(|(check, _)| check), - Some(Check::Tls) - ); - assert_eq!( - report.outcome(Check::DidDocument), - &Outcome::Skipped { after: Check::Tls } - ); - } - - /// A server answering HTTPS with a document for somebody else's name. - #[tokio::test] - async fn a_document_naming_another_host_fails_the_document_check() { - let report = preflight( - &Resolver(Ok(addr())), - &Fetcher(Ok(document("someone-else.example.test"))), - &Describer(Ok("did:web:pds.example.test".into())), - "pds.example.test", - ) - .await; - assert!(report.outcome(Check::Tls).passed(), "TLS did answer"); - assert_eq!( - report.first_failure().map(|(check, _)| check), - Some(Check::DidDocument) - ); - } - - /// The failure that is invisible from either server involved: two - /// deployments answering one name. - #[tokio::test] - async fn a_describe_server_mismatch_is_its_own_failure() { - let host = "pds.example.test"; - let report = preflight( - &Resolver(Ok(addr())), - &Fetcher(Ok(document(host))), - &Describer(Ok("did:web:a-different-deployment.example.test".into())), - host, - ) - .await; - assert!(report.outcome(Check::DidDocument).passed()); - let (check, reason) = report.first_failure().expect("a failure"); - assert_eq!(check, Check::ServerDescription); - assert!(reason.contains("a-different-deployment"), "{reason}"); - } - - #[tokio::test] - async fn the_report_renders_every_check_in_order() { - let host = "pds.example.test"; - let report = preflight( - &Resolver(Ok(addr())), - &Fetcher(Ok(document(host))), - &Describer(Ok(format!("did:web:{host}"))), - host, - ) - .await; - let rendered = report.to_string(); - let mut cursor = 0; - for check in Check::ALL { - let at = rendered - .find(check.as_str()) - .unwrap_or_else(|| panic!("{check} is missing from the report:\n{rendered}")); - assert!(at >= cursor, "{check} is out of order:\n{rendered}"); - cursor = at; - } + run: run_steps(env, &Target::new(hostname), &CLAIM_STEPS).await, } } diff --git a/crates/didbot-fsm/src/lib.rs b/crates/didbot-fsm/src/lib.rs index af3175ca..a97e80d9 100644 --- a/crates/didbot-fsm/src/lib.rs +++ b/crates/didbot-fsm/src/lib.rs @@ -47,13 +47,13 @@ //! //! An earlier version of this crate carried a three-valued precondition //! type, then dropped it: the lifecycle that wanted it blocked on DNS, TLS -//! and its own `did.json`, those checks moved to `didbot_claim::preflight` -//! on the operator's machine, and `preflight` runs them synchronously and -//! needs only two values. +//! and its own `did.json`, those checks moved to `didbot_onboarding` on the +//! operator's machine, where a run is synchronous and a check is answered or +//! waiting on a step below it. //! //! `didbot_reconcile::ZoneReconciler` is the caller that brings the concept -//! back, and it needs the third value for a reason `preflight` does not -//! have: it *acts*. A repair it has not attempted yet and a repair it +//! back, and it needs the third value for a reason an onboarding run does +//! not have: it *acts*. A repair it has not attempted yet and a repair it //! attempted and could not make are different facts about the zone, and //! collapsing them makes the first tick of a healthy process //! indistinguishable from an outage — which is exactly the input its @@ -527,9 +527,9 @@ impl fmt::Display for Gate { /// causal order — read the zone before comparing it, compare it before /// repairing it — and one failure reports as one cause rather than as every /// downstream gate it made unanswerable. This is the same shape -/// `didbot_claim::preflight` uses when it skips the checks below a failed -/// one; the difference is that this set persists across ticks, which is why -/// it needs a value for "not attempted yet." +/// `didbot_onboarding` uses when it blocks the steps below a failed one; the +/// difference is that this set persists across ticks, which is why it needs +/// a value for "not attempted yet." /// /// A gate is addressed by its `&'static str` name, which the caller owns. /// Naming one that was not declared is a caller bug rather than a runtime diff --git a/crates/didbot-onboarding/tests/steps.rs b/crates/didbot-onboarding/tests/steps.rs index 0456fe46..f596b2e6 100644 --- a/crates/didbot-onboarding/tests/steps.rs +++ b/crates/didbot-onboarding/tests/steps.rs @@ -27,8 +27,14 @@ enum Broken { Host, /// The certificate expired. Certificate, + /// The name exists and publishes no address. + ZoneNoAddresses, /// The document names somebody else. Document, + /// The document publishes no `#atproto` key at all. + DocumentMissingKey, + /// It publishes one that is not a key atproto verifies commits with. + DocumentUnusableKey, /// Neither handle record answers. Handle, /// `describeServer` names another deployment. @@ -47,10 +53,12 @@ impl Broken { /// The step this breakage is meant to fail. fn step(self) -> Step { match self { - Broken::Zone => Step::Zone, + Broken::Zone | Broken::ZoneNoAddresses => Step::Zone, Broken::Host => Step::Host, Broken::Certificate => Step::Certificate, - Broken::Document => Step::Document, + Broken::Document | Broken::DocumentMissingKey | Broken::DocumentUnusableKey => { + Step::Document + } Broken::Handle => Step::Handle, Broken::Description => Step::Description, Broken::Identity => Step::Identity, @@ -60,11 +68,14 @@ impl Broken { } } - const ALL: [Broken; 10] = [ + const ALL: [Broken; 13] = [ Broken::Zone, + Broken::ZoneNoAddresses, Broken::Host, Broken::Certificate, Broken::Document, + Broken::DocumentMissingKey, + Broken::DocumentUnusableKey, Broken::Handle, Broken::Description, Broken::Identity, @@ -107,23 +118,36 @@ impl Fixture { } else { DID }; - serde_json::json!({ + let mut value = serde_json::json!({ "@context": ["https://www.w3.org/ns/did/v1"], "id": did, "alsoKnownAs": [format!("at://{HOST}")], - "verificationMethod": [{ - "id": format!("{did}#atproto"), - "type": "Multikey", - "controller": did, - "publicKeyMultibase": self.key, - }], + "verificationMethod": [], "service": [{ "id": format!("{did}#atproto_pds"), "type": "AtprotoPersonalDataServer", "serviceEndpoint": format!("https://{HOST}"), }], - }) - .to_string() + }); + // The claimed server's document is its own answer, so the key in it + // is whatever it chose to serve. A string that is not an atproto + // public key has to refuse rather than be reported claimable: + // `didbot_pds::Ownership` compares `subjectKey` for exact equality + // against the key the server really holds. + let key = match self.broken { + Some(Broken::DocumentMissingKey) => None, + Some(Broken::DocumentUnusableKey) => Some("zNotARealKeyAtAll"), + _ => Some(self.key.as_str()), + }; + if let Some(key) = key { + value["verificationMethod"] = serde_json::json!([{ + "id": format!("{did}#atproto"), + "type": "Multikey", + "controller": did, + "publicKeyMultibase": key, + }]); + } + value.to_string() } fn status(&self) -> String { @@ -198,6 +222,7 @@ impl Environment for Fixture { RecordKind::Address if self.is(Broken::Zone) => { Err(Unavailable::failed("NXDOMAIN: the name does not exist")) } + RecordKind::Address if self.is(Broken::ZoneNoAddresses) => Ok(Vec::new()), RecordKind::Address => Ok(vec!["203.0.113.7".to_owned()]), // A zone cut one label above the deployment's own name, which is // where a real delegation sits. diff --git a/crates/didbot-pds/src/server_state.rs b/crates/didbot-pds/src/server_state.rs index 417bef6b..f93e5151 100644 --- a/crates/didbot-pds/src/server_state.rs +++ b/crates/didbot-pds/src/server_state.rs @@ -70,8 +70,8 @@ //! DNS, TLS and `did.json` and blocked on them. That was the wrong vantage //! point: a server can only confirm what it *wrote*, and what matters is //! whether anyone else can see it. Those checks belong where somebody is -//! actually looking from, which is the operator's machine — -//! `didbot_claim::preflight`. +//! actually looking from, which is the operator's machine and the policy +//! page — `didbot_onboarding`. //! //! **DNS reconciliation.** Keeping a zone this deployment manages correct is //! a standing obligation in every state, not a bootstrap step: if a record diff --git a/crates/didbot-reconcile/src/lib.rs b/crates/didbot-reconcile/src/lib.rs index c7ebbc29..21802595 100644 --- a/crates/didbot-reconcile/src/lib.rs +++ b/crates/didbot-reconcile/src/lib.rs @@ -11,9 +11,9 @@ //! //! # What it is, against what `didbot-claim` is //! -//! `didbot_claim::preflight` **observes from outside**: it runs on the -//! operator's machine, resolves through a real resolver, and reports. It -//! exists because a server checking its own DNS confirms only what it wrote. +//! `didbot_onboarding` **observes from outside**: it runs on the operator's +//! machine, resolves through a real resolver, and reports. It exists because +//! a server checking its own DNS confirms only what it wrote. //! //! This **repairs from inside**, which is the thing no outside observer can //! do, and it is honest about the limit that makes the two complementary: diff --git a/docs/server-lifecycle.md b/docs/server-lifecycle.md index cf64628b..ef001022 100644 --- a/docs/server-lifecycle.md +++ b/docs/server-lifecycle.md @@ -117,9 +117,10 @@ burst multiply outbound requests, which is the wrong quantity to bound. The server knows two things about itself and checks nothing else: whether it holds its own signing key, and whether its own poll found a claim. -Everything external — DNS resolving, TLS terminating, the `did:web` document -serving, `describeServer` agreeing — is checked by `didbot_claim::preflight`, -on the operator's own machine, and reported by `didbot-claim --check`. A +Everything external — the zone delegated, DNS resolving, TLS terminating, the +`did:web` document serving, `describeServer` agreeing — is checked by +`didbot_onboarding`, from wherever somebody is looking: the operator's own +machine through `didbot-claim --check`, or the policy page. A server checking its own DNS confirms what it wrote; a server checking its own TLS is often checking a loopback path; a server checking its own `did.json` proves it can talk to itself, and the failure worth catching (a *different* -- 2.51.2