From 1edd51235e96c184d914d4e019c9ccfc71b997bd Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 20:38:02 -0500 Subject: [PATCH 1/4] fix(control): validate registration responses into an exhaustive state machine RegisterResponse modeled only AuthURL/MachineAuthorized/NodeKeyExpiry (the last of which the reference protocol never sends), and register()/ poll_registration() inferred outcome from AuthURL alone: any present URL (including "") became NeedsAuth, and !MachineAuthorized with no URL fell through to Authorized. Error was never deserialized, so a rejection with an empty AuthURL was silently accepted as the empty-URL auth flow. Add Error and NodeKeyExpired to the wire DTO; drop the nonexistent NodeKeyExpiry. classify_register_response() validates every reachable field combination into one RegisterOutcome variant (Authorized, NeedsAuth over a non-empty-validated NonEmptyUrl, RotateNodeKey, Rejected, or Contradictory(RegisterFault) for the two combinations the protocol does not allow), with Error taking precedence over NodeKeyExpired over MachineAuthorized/AuthURL. poll_registration() now runs the same classifier as register() instead of returning the raw response unvalidated. --- crates/dictyon/examples/connect.rs | 35 ++- crates/dictyon/src/control/mod.rs | 103 ++++--- crates/dictyon/src/control/register.rs | 343 ++++++++++++++++++++++ crates/dictyon/tests/register_flow/mod.rs | 119 ++++++++ crates/dictyon/tests/wire_integration.rs | 5 + crates/mitos/src/types/mod.rs | 31 +- crates/mitos/tests/public_api.rs | 33 ++- 7 files changed, 610 insertions(+), 59 deletions(-) create mode 100644 crates/dictyon/src/control/register.rs create mode 100644 crates/dictyon/tests/register_flow/mod.rs diff --git a/crates/dictyon/examples/connect.rs b/crates/dictyon/examples/connect.rs index 69fc2a8..08822a4 100644 --- a/crates/dictyon/examples/connect.rs +++ b/crates/dictyon/examples/connect.rs @@ -134,25 +134,38 @@ async fn register_node( auth_key: Option<&str>, ) -> Result<(), ExampleError> { info!("registering…"); - match client.register(stream, auth_key).await? { + report_register_outcome(client.register(stream, auth_key).await?); + Ok(()) +} + +/// Log what a [`RegisterOutcome`] means for this example run. +/// +/// A real caller (not this example) would poll again after `NeedsAuth` +/// once the user has visited the URL, and would retry registration with a +/// fresh node key after `RotateNodeKey`; this example only demonstrates +/// that every outcome is a distinguishable, handled case. +fn report_register_outcome(outcome: RegisterOutcome) { + match outcome { RegisterOutcome::Authorized(resp) => { - info!( - authorized = resp.machine_authorized, - expiry = ?resp.node_key_expiry, - "node authorized" - ); + info!(authorized = resp.machine_authorized, "node authorized"); } - RegisterOutcome::NeedsAuth { auth_url } => { - info!("visit to authorize: {auth_url}"); - let resp = client.poll_registration(stream, &auth_url).await?; - info!(authorized = resp.machine_authorized, "auth complete"); + RegisterOutcome::NeedsAuth(url) => { + info!("visit to authorize: {url}"); + } + RegisterOutcome::RotateNodeKey => { + warn!("server reports the node key has expired; a fresh key is required"); + } + RegisterOutcome::Rejected { reason } => { + warn!(reason, "server rejected the registration request"); + } + RegisterOutcome::Contradictory(fault) => { + warn!(?fault, "server sent a response the protocol does not allow"); } // RegisterOutcome is #[non_exhaustive]; cover future variants. _ => { warn!("unknown register outcome variant; treating as unsupported"); } } - Ok(()) } async fn stream_map( diff --git a/crates/dictyon/src/control/mod.rs b/crates/dictyon/src/control/mod.rs index 5db2b4a..0f6f00d 100644 --- a/crates/dictyon/src/control/mod.rs +++ b/crates/dictyon/src/control/mod.rs @@ -30,6 +30,11 @@ use tracing::{debug, warn}; use crate::transport::ControlConnection; use crate::wire::AsyncControlStream; +mod register; + +use register::classify_register_response; +pub use register::{NonEmptyUrl, RegisterFault, RegisterOutcome}; + /// Upper bound on the peers a netmap retains from a coordination server. /// /// WHY: every peer-bearing field of a [`MapResponse`] is server-controlled and @@ -99,22 +104,6 @@ impl From for ControlError { } } -/// Outcome of an async registration attempt. -/// -/// When the machine already has a valid pre-auth key, the server -/// authorizes it immediately ([`RegisterOutcome::Authorized`]). Otherwise, -/// the user must visit an auth URL ([`RegisterOutcome::NeedsAuth`]). -#[non_exhaustive] -pub enum RegisterOutcome { - /// The node was authorized; contains the server's registration response. - Authorized(RegisterResponse), - /// Interactive auth is required; the user must visit this URL. - NeedsAuth { - /// URL the user must visit to authorize this node. - auth_url: String, - }, -} - /// The local view of the network map, maintained by applying /// [`MapResponse`] updates. /// @@ -307,13 +296,18 @@ impl ControlClient { /// Register this node asynchronously using an [`AsyncControlStream`]. /// - /// Serializes a [`RegisterRequest`], sends it over the stream, and parses - /// the response. Returns either an authorized [`RegisterResponse`] or the - /// auth URL the user must visit. + /// Serializes a [`RegisterRequest`], sends it over the stream, and + /// validates the response into a [`RegisterOutcome`] -- every field + /// combination the server can send, including ones the protocol does + /// not allow, lands in a variant of that type. /// /// # Errors /// - /// Returns [`ControlError`] on serialization, I/O, or parse failure. + /// Returns [`ControlError`] on serialization, I/O, or a JSON payload + /// that fails to parse. A syntactically valid response the protocol + /// still rejects -- an explicit rejection, contradictory fields, an + /// expired key -- is not an error here; it is a [`RegisterOutcome`] + /// variant, because the request itself succeeded. pub async fn register( &mut self, stream: &mut AsyncControlStream, @@ -330,28 +324,18 @@ impl ControlClient { let raw = stream.recv_message().await?; let resp = parse_register_response(&raw)?; - - if let Some(url) = resp.auth_url.clone() { - debug!( - target: "dictyon::control", - outcome = "needs_auth", - "register requires interactive auth", - ); - Ok(RegisterOutcome::NeedsAuth { auth_url: url }) - } else { - debug!( - target: "dictyon::control", - outcome = "authorized", - "register authorized", - ); - Ok(RegisterOutcome::Authorized(resp)) - } + let outcome = classify_register_response(resp); + log_register_outcome(&outcome); + Ok(outcome) } /// Poll for registration completion after the user has visited the auth URL. /// /// Sends a new [`RegisterRequest`] with the `followup` field set to the - /// URL returned in the initial response, and waits for authorization. + /// URL returned in the initial response, and validates the response + /// into a [`RegisterOutcome`] via the same rules [`Self::register`] + /// uses -- a followup poll can be rejected or come back contradictory + /// exactly as the initial request can. /// /// # Errors /// @@ -360,7 +344,7 @@ impl ControlClient { &mut self, stream: &mut AsyncControlStream, followup_url: &str, - ) -> Result { + ) -> Result { debug!( target: "dictyon::control", followup_url, @@ -378,7 +362,10 @@ impl ControlClient { stream.send_message(&framed).await?; let raw = stream.recv_message().await?; - parse_register_response(&raw) + let resp = parse_register_response(&raw)?; + let outcome = classify_register_response(resp); + log_register_outcome(&outcome); + Ok(outcome) } /// Send the initial map request and start streaming map updates. @@ -706,6 +693,44 @@ fn parse_register_response(raw: &[u8]) -> Result }) } +/// Log a validated [`RegisterOutcome`] at a level matching how much it +/// deserves operator attention. +/// +/// WHY: [`RegisterOutcome::Rejected`], `RotateNodeKey`, and `Contradictory` +/// are exactly the shapes this issue exists to stop silently tolerating, so +/// they log at `warn` -- a caller polling in a loop should not need to +/// inspect every outcome to notice the control server is sending something +/// it shouldn't. +fn log_register_outcome(outcome: &RegisterOutcome) { + match outcome { + RegisterOutcome::Authorized(_) => { + debug!(target: "dictyon::control", outcome = "authorized", "register authorized"); + } + RegisterOutcome::NeedsAuth(url) => { + debug!( + target: "dictyon::control", + outcome = "needs_auth", + auth_url = %url, + "register requires interactive auth", + ); + } + RegisterOutcome::RotateNodeKey => { + warn!(target: "dictyon::control", outcome = "rotate_node_key", "server reports the node key has expired"); + } + RegisterOutcome::Rejected { reason } => { + warn!(target: "dictyon::control", outcome = "rejected", reason = %reason, "server rejected the registration request"); + } + RegisterOutcome::Contradictory(fault) => { + warn!( + target: "dictyon::control", + outcome = "contradictory", + fault = ?fault, + "server sent a registration response the protocol does not allow", + ); + } + } +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- diff --git a/crates/dictyon/src/control/register.rs b/crates/dictyon/src/control/register.rs new file mode 100644 index 0000000..af3e27b --- /dev/null +++ b/crates/dictyon/src/control/register.rs @@ -0,0 +1,343 @@ +//! Domain validation for `POST /machine/register` responses. +//! +//! [`classify_register_response`] is the single point where a wire +//! [`RegisterResponse`] becomes a caller-facing [`RegisterOutcome`]. Every +//! reachable combination of `Error`, `NodeKeyExpired`, `MachineAuthorized`, +//! and `AuthURL` maps to exactly one variant, including combinations the +//! control server should never send. Nothing is inferred from a single +//! field in isolation, and nothing is silently coerced into a variant its +//! fields do not support. + +use std::fmt; + +use mitos::types::RegisterResponse; + +/// Domain-validated outcome of a registration attempt. +/// +/// Every [`RegisterResponse`] classifies into exactly one variant via +/// [`classify_register_response`]: there is no wire response this type +/// cannot represent, and no variant that tolerates a response shape it +/// should reject. +#[derive(Debug)] +#[non_exhaustive] +pub enum RegisterOutcome { + /// The node is authorized. Carries the full response for callers that + /// need node-key or other server-reported detail. + Authorized(RegisterResponse), + /// Interactive auth is required at a server-supplied, non-empty URL. + NeedsAuth(NonEmptyUrl), + /// The node key has expired; a fresh key must be registered before any + /// authorization claim in this response can be trusted. + RotateNodeKey, + /// The server explicitly rejected the request. + Rejected { + /// The server-supplied rejection reason. + reason: String, + }, + /// The response combined fields in a way the protocol does not allow. + /// See [`RegisterFault`] for which combination. + Contradictory(RegisterFault), +} + +/// The specific way a [`RegisterResponse`] failed to validate into a +/// non-contradictory [`RegisterOutcome`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum RegisterFault { + /// `MachineAuthorized: true` alongside a non-empty `AuthURL`: the + /// server cannot claim the node is authorized while also demanding + /// interactive auth for it. + AuthorizedWithPendingUrl, + /// `MachineAuthorized: false` with no `AuthURL`, `Error`, or + /// `NodeKeyExpired` set: the server declined to authorize the node + /// without stating why or what the caller should do next. + UnauthorizedWithoutExplanation, +} + +/// An `AuthURL` value validated to be non-empty. +/// +/// WHY: an empty `AuthURL` is not a URL the user can visit. The reference +/// server marshals "no URL" as `""` rather than omitting the field, so a +/// naive `Option` check tolerates that case as if it were a real +/// URL. Wrapping the validated string keeps [`RegisterOutcome::NeedsAuth`] +/// from ever representing the empty-string case -- the type itself is the +/// proof the value was checked, and there is no public constructor, so a +/// [`NonEmptyUrl`] can only exist by having passed +/// [`classify_register_response`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct NonEmptyUrl(String); + +impl NonEmptyUrl { + /// Borrow the validated URL. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl fmt::Display for NonEmptyUrl { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +/// Validate a wire [`RegisterResponse`] into an exhaustive [`RegisterOutcome`]. +/// +/// Precedence, first match wins: +/// +/// 1. A non-empty `Error` always rejects, regardless of the other fields -- +/// an explicit rejection reason is never shadowed by a stray +/// `MachineAuthorized` or `AuthURL` value. +/// 2. `NodeKeyExpired` requires rotation before any authorization claim in +/// the same response can be trusted. +/// 3. Otherwise `MachineAuthorized` and a non-empty `AuthURL` combine: +/// authorized-with-no-url is [`RegisterOutcome::Authorized`], +/// unauthorized-with-a-url is [`RegisterOutcome::NeedsAuth`], and the two +/// remaining combinations -- authorized-with-a-url and +/// unauthorized-with-no-url -- are [`RegisterOutcome::Contradictory`]. +/// +/// A present but empty `Error` or `AuthURL` is treated as absent: the +/// reference control server marshals both as `""` rather than omitting +/// them, so an empty string carries no information distinct from the field +/// being unset. +pub(super) fn classify_register_response(resp: RegisterResponse) -> RegisterOutcome { + if let Some(reason) = non_empty(resp.error.as_deref()) { + return RegisterOutcome::Rejected { + reason: reason.to_string(), + }; + } + + if resp.node_key_expired { + return RegisterOutcome::RotateNodeKey; + } + + let url = non_empty(resp.auth_url.as_deref()).map(|url| NonEmptyUrl(url.to_string())); + match (resp.machine_authorized, url) { + (true, None) => RegisterOutcome::Authorized(resp), + (true, Some(_)) => RegisterOutcome::Contradictory(RegisterFault::AuthorizedWithPendingUrl), + (false, Some(url)) => RegisterOutcome::NeedsAuth(url), + (false, None) => { + RegisterOutcome::Contradictory(RegisterFault::UnauthorizedWithoutExplanation) + } + } +} + +/// Returns `s` unless it is empty. +fn non_empty(s: Option<&str>) -> Option<&str> { + s.filter(|value| !value.is_empty()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A [`RegisterResponse`] with every field at its "nothing set" value, + /// so each test overrides only the fields its case is about. + fn blank_response() -> RegisterResponse { + RegisterResponse { + auth_url: None, + machine_authorized: false, + node_key_expired: false, + error: None, + } + } + + #[test] + fn authorized_with_no_url_is_authorized() { + let resp = RegisterResponse { + machine_authorized: true, + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + assert!( + matches!(outcome, RegisterOutcome::Authorized(_)), + "expected Authorized, got {outcome:?}" + ); + } + + /// WHY(#66): the empty-string `AuthURL` case is the exact shape a + /// PascalCase Go server sends alongside `MachineAuthorized: true` -- + /// the field is present because the struct has no `omitempty` tag, not + /// because interactive auth is actually needed. It must classify + /// identically to the field being absent. + #[test] + fn authorized_with_empty_url_is_authorized_not_needs_auth() { + let resp = RegisterResponse { + machine_authorized: true, + auth_url: Some(String::new()), + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + assert!( + matches!(outcome, RegisterOutcome::Authorized(_)), + "an empty AuthURL alongside MachineAuthorized:true must not become NeedsAuth, got {outcome:?}" + ); + } + + #[test] + fn unauthorized_with_url_needs_auth() { + let resp = RegisterResponse { + machine_authorized: false, + auth_url: Some("https://login.tailscale.com/a/abc".to_string()), + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + match outcome { + RegisterOutcome::NeedsAuth(url) => { + assert_eq!(url.as_str(), "https://login.tailscale.com/a/abc"); + } + other => panic!("expected NeedsAuth, got {other:?}"), + } + } + + /// WHY(#66): this used to discard the rejection and fall into the + /// empty-URL auth flow. `Error` must win over `MachineAuthorized` and + /// `AuthURL` unconditionally. + #[test] + fn error_rejects_even_with_empty_url_and_unauthorized() { + let resp = RegisterResponse { + machine_authorized: false, + auth_url: Some(String::new()), + error: Some("invalid auth key".to_string()), + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + match outcome { + RegisterOutcome::Rejected { reason } => assert_eq!(reason, "invalid auth key"), + other => panic!("expected Rejected, got {other:?}"), + } + } + + /// Error takes precedence even over a claimed authorization -- a + /// contradictory server should never have the "good" half of its + /// response believed. + #[test] + fn error_rejects_even_when_machine_authorized_is_true() { + let resp = RegisterResponse { + machine_authorized: true, + error: Some("account suspended".to_string()), + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + assert!( + matches!(outcome, RegisterOutcome::Rejected { reason } if reason == "account suspended"), + "expected Rejected, got {outcome:?}" + ); + } + + /// A present-but-empty `Error` carries no information, matching how the + /// reference server marshals its zero value. + #[test] + fn empty_error_string_does_not_reject() { + let resp = RegisterResponse { + machine_authorized: true, + error: Some(String::new()), + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + assert!( + matches!(outcome, RegisterOutcome::Authorized(_)), + "an empty Error string must not reject, got {outcome:?}" + ); + } + + #[test] + fn node_key_expired_requests_rotation() { + let resp = RegisterResponse { + node_key_expired: true, + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + assert!( + matches!(outcome, RegisterOutcome::RotateNodeKey), + "expected RotateNodeKey, got {outcome:?}" + ); + } + + /// Rotation is required before an authorization claim in the same + /// response can be trusted, so it takes precedence over + /// `MachineAuthorized: true`. + #[test] + fn node_key_expired_wins_over_machine_authorized() { + let resp = RegisterResponse { + node_key_expired: true, + machine_authorized: true, + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + assert!( + matches!(outcome, RegisterOutcome::RotateNodeKey), + "expected RotateNodeKey, got {outcome:?}" + ); + } + + /// WHY(#66): this used to become `Authorized` by inferring success from + /// an absent `AuthURL` alone, discarding `MachineAuthorized: false`. + #[test] + fn unauthorized_with_no_signal_is_contradictory_not_authorized() { + let resp = blank_response(); + + let outcome = classify_register_response(resp); + + assert!( + matches!( + outcome, + RegisterOutcome::Contradictory(RegisterFault::UnauthorizedWithoutExplanation) + ), + "expected Contradictory(UnauthorizedWithoutExplanation), got {outcome:?}" + ); + } + + /// A server cannot claim both "already authorized" and "here is a URL + /// to authorize at" -- neither half of that claim should be believed + /// silently. + #[test] + fn authorized_with_pending_url_is_contradictory() { + let resp = RegisterResponse { + machine_authorized: true, + auth_url: Some("https://login.tailscale.com/a/abc".to_string()), + ..blank_response() + }; + + let outcome = classify_register_response(resp); + + assert!( + matches!( + outcome, + RegisterOutcome::Contradictory(RegisterFault::AuthorizedWithPendingUrl) + ), + "expected Contradictory(AuthorizedWithPendingUrl), got {outcome:?}" + ); + } + + #[test] + fn non_empty_url_display_matches_as_str() { + let resp = RegisterResponse { + machine_authorized: false, + auth_url: Some("https://login.tailscale.com/a/xyz".to_string()), + ..blank_response() + }; + + let RegisterOutcome::NeedsAuth(url) = classify_register_response(resp) else { + panic!("expected NeedsAuth"); + }; + + assert_eq!(url.to_string(), url.as_str()); + } +} diff --git a/crates/dictyon/tests/register_flow/mod.rs b/crates/dictyon/tests/register_flow/mod.rs new file mode 100644 index 0000000..2f5ecac --- /dev/null +++ b/crates/dictyon/tests/register_flow/mod.rs @@ -0,0 +1,119 @@ +//! Followup-poll registration flow, over a real Noise-encrypted connection. +//! +//! Split into a sibling file because `wire_integration.rs` + this test +//! exceeded the `RUST/file-too-long` threshold. + +use super::*; + +/// WHY(#66): `poll_registration` runs the same wire round-trip as +/// `register` and must validate its response through the same exhaustive +/// classifier -- an unvalidated followup response was the original defect's +/// second, untested entry point. This drives `register` to `NeedsAuth` and +/// then the followup to `Rejected`, over one real Noise-encrypted +/// connection, proving the rejection is not discarded on the polled path +/// either. +#[tokio::test] +#[expect( + clippy::expect_used, + reason = "integration tests use expect to keep fixture failures explicit" +)] +async fn poll_registration_validates_the_followup_response_into_an_outcome() { + let (server_tls_cfg, client_tls_cfg) = make_test_tls_pair(); + let keys = Arc::new(MockServerKeys::generate()); + + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind listener"); + let addr = listener.local_addr().expect("local_addr"); + let acceptor = TlsAcceptor::from(Arc::new(server_tls_cfg)); + + let keys_clone = Arc::clone(&keys); + let server = tokio::spawn(async move { + // Connection 1: key fetch. + let (tcp1, _) = listener.accept().await.expect("accept key conn"); + let mut tls1 = acceptor.accept(tcp1).await.expect("tls accept key conn"); + handle_key_request(&mut tls1, &keys_clone).await; + + // Connection 2: Noise upgrade + register RPC + followup poll RPC. + let (tcp2, _) = listener.accept().await.expect("accept noise conn"); + let mut tls2 = acceptor.accept(tcp2).await.expect("tls accept noise conn"); + let mut transport = handle_noise_upgrade(&mut tls2, &keys_clone).await; + + // First register request: respond NeedsAuth. + let ciphertext = read_noise_frame(&mut tls2).await; + let mut plaintext_buf = vec![0u8; ciphertext.len()]; + let pt_len = transport + .read_message(&ciphertext, &mut plaintext_buf) + .expect("decrypt register request"); + let json_payload = control_payload(&plaintext_buf[..pt_len], "register request"); + let req: serde_json::Value = + serde_json::from_slice(json_payload).expect("register request should be valid JSON"); + assert!( + req.get("Followup").is_none(), + "initial register request should carry no Followup" + ); + + let needs_auth_json = + br#"{"MachineAuthorized":false,"AuthURL":"https://login.tailscale.com/a/xyz"}"#; + write_noise_frame(&mut tls2, &mut transport, needs_auth_json).await; + tls2.flush().await.expect("flush after register response"); + + // Followup poll request: respond Rejected. + let ciphertext = read_noise_frame(&mut tls2).await; + let mut plaintext_buf = vec![0u8; ciphertext.len()]; + let pt_len = transport + .read_message(&ciphertext, &mut plaintext_buf) + .expect("decrypt followup request"); + let json_payload = control_payload(&plaintext_buf[..pt_len], "followup request"); + let req: serde_json::Value = + serde_json::from_slice(json_payload).expect("followup request should be valid JSON"); + assert_eq!( + req.get("Followup").and_then(serde_json::Value::as_str), + Some("https://login.tailscale.com/a/xyz"), + "followup request should carry the NeedsAuth URL" + ); + + let rejected_json = + br#"{"Error":"invalid auth key","MachineAuthorized":false,"AuthURL":""}"#; + write_noise_frame(&mut tls2, &mut transport, rejected_json).await; + tls2.flush().await.expect("flush after followup response"); + }); + + let machine_key = MachinePrivate::generate(); + let node_key = mitos::keys::NodePrivate::generate(); + let disco_key = mitos::keys::DiscoPrivate::generate(); + + let config = dictyon::wire::ControlConfig::new( + format!("https://127.0.0.1:{}", addr.port()), + MachinePrivate::from_bytes(*machine_key.as_bytes()), + ); + + let mut stream = dictyon::wire::connect_with_tls(&config, client_tls_cfg) + .await + .expect("connect should succeed"); + + let (conn, ()) = make_dummy_transport(); + let mut client = dictyon::control::ControlClient::new(conn, machine_key, node_key, disco_key); + + let first = client + .register(&mut stream, None) + .await + .expect("register should succeed"); + let dictyon::control::RegisterOutcome::NeedsAuth(url) = first else { + panic!("expected NeedsAuth from the initial register"); + }; + assert_eq!(url.as_str(), "https://login.tailscale.com/a/xyz"); + + let followup = client + .poll_registration(&mut stream, url.as_str()) + .await + .expect("poll_registration should succeed"); + match followup { + dictyon::control::RegisterOutcome::Rejected { reason } => { + assert_eq!(reason, "invalid auth key"); + } + other => panic!("expected Rejected from the followup poll, got {other:?}"), + } + + server.await.expect("mock server should not panic"); +} diff --git a/crates/dictyon/tests/wire_integration.rs b/crates/dictyon/tests/wire_integration.rs index 4f6bd19..e8d609b 100644 --- a/crates/dictyon/tests/wire_integration.rs +++ b/crates/dictyon/tests/wire_integration.rs @@ -471,6 +471,11 @@ async fn register_returns_authorized_with_preauth_key() { server.await.expect("mock server should not panic"); } +// WHY: split into a sibling file (see its own doc comment) because this +// test plus the rest of the file exceeded the `RUST/file-too-long` +// threshold. `use super::*;` there reaches every helper defined below. +mod register_flow; + #[tokio::test] #[expect( clippy::expect_used, diff --git a/crates/mitos/src/types/mod.rs b/crates/mitos/src/types/mod.rs index 03918df..f2b9754 100644 --- a/crates/mitos/src/types/mod.rs +++ b/crates/mitos/src/types/mod.rs @@ -129,21 +129,36 @@ pub struct Hostinfo { } /// Response from `POST /machine/register`. +/// +/// A raw wire DTO: it mirrors the control server's JSON fields as closely +/// as `serde(rename)` allows and carries no validation of its own. Field +/// *combinations* the protocol does not allow (e.g. `machine_authorized: +/// true` alongside a non-empty `auth_url`) are representable here on +/// purpose -- rejecting them is the consuming client's job (`dictyon`'s +/// `control::classify_register_response`), once every field the server can +/// set is actually deserialized. This type must not grow validation logic; +/// see `RUST.md` on wire-DTO exemptions. #[derive(Debug, Deserialize)] pub struct RegisterResponse { - /// URL the user must visit to authorize this machine. `None` if the - /// machine is already authorized (e.g. via pre-auth key). - #[serde(rename = "AuthURL")] + /// URL the user must visit to authorize this machine. `None` or an + /// empty string both mean "no URL" -- the reference server marshals a + /// zero-value string as `""` rather than omitting the field. + #[serde(rename = "AuthURL", default)] pub auth_url: Option, /// Whether the machine is now authorized. - #[serde(rename = "MachineAuthorized")] + #[serde(rename = "MachineAuthorized", default)] pub machine_authorized: bool, - /// ISO 8601 expiry timestamp for the node key. `None` if the key does - /// not expire. - #[serde(rename = "NodeKeyExpiry")] - pub node_key_expiry: Option, + /// Whether the node key has expired and must be rotated before the + /// server will consider this node authorized. + #[serde(rename = "NodeKeyExpired", default)] + pub node_key_expired: bool, + + /// Server-supplied reason the request was rejected. `None` or an empty + /// string both mean "no error", for the same reason as `auth_url`. + #[serde(rename = "Error", default)] + pub error: Option, } // --------------------------------------------------------------------------- diff --git a/crates/mitos/tests/public_api.rs b/crates/mitos/tests/public_api.rs index 5fc7875..3a4111d 100644 --- a/crates/mitos/tests/public_api.rs +++ b/crates/mitos/tests/public_api.rs @@ -200,5 +200,36 @@ fn register_response_parses_auth_url_variant() { Some("https://login.tailscale.com/a/abc") ); assert!(!resp.machine_authorized); - assert!(resp.node_key_expiry.is_none()); + assert!(!resp.node_key_expired); + assert!(resp.error.is_none()); +} + +/// The reference control server's full `RegisterResponse` shape includes +/// `Error` and `NodeKeyExpired` alongside `MachineAuthorized` and +/// `AuthURL` -- all four must reach the public API, not just the two this +/// crate originally modeled. +#[test] +fn register_response_parses_error_and_node_key_expired() { + let json = r#"{ + "Error": "invalid auth key", + "MachineAuthorized": false, + "NodeKeyExpired": true + }"#; + let resp: RegisterResponse = serde_json::from_str(json).expect("full variant parses"); + assert_eq!(resp.error.as_deref(), Some("invalid auth key")); + assert!(resp.node_key_expired); + assert!(!resp.machine_authorized); + assert!(resp.auth_url.is_none()); +} + +/// The reference server marshals its zero values (`""`, `false`) rather +/// than omitting fields; a response naming none of them must still parse, +/// with every field at its absent/false default. +#[test] +fn register_response_missing_fields_default_to_absent() { + let resp: RegisterResponse = serde_json::from_str("{}").expect("empty object parses"); + assert!(resp.auth_url.is_none()); + assert!(!resp.machine_authorized); + assert!(!resp.node_key_expired); + assert!(resp.error.is_none()); } From c59c247e2bc89683491debfbfa09413c1c7ff1bd Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 20:40:54 -0500 Subject: [PATCH 2/4] fix(control): avoid partial-move of RegisterOutcome in a register.rs test matches!() with a struct-variant binding moves the bound field out of the scrutinee; reusing the scrutinee in the assert! message afterward is an E0382 partial-move. Match explicitly instead, matching the pattern already used by the sibling Rejected tests in this file. --- crates/dictyon/src/control/register.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/dictyon/src/control/register.rs b/crates/dictyon/src/control/register.rs index af3e27b..d262377 100644 --- a/crates/dictyon/src/control/register.rs +++ b/crates/dictyon/src/control/register.rs @@ -229,10 +229,10 @@ mod tests { let outcome = classify_register_response(resp); - assert!( - matches!(outcome, RegisterOutcome::Rejected { reason } if reason == "account suspended"), - "expected Rejected, got {outcome:?}" - ); + match outcome { + RegisterOutcome::Rejected { reason } => assert_eq!(reason, "account suspended"), + other => panic!("expected Rejected, got {other:?}"), + } } /// A present-but-empty `Error` carries no information, matching how the From 5f70d9629c6b788f09ecffc5babe451371f8a42e Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 20:46:43 -0500 Subject: [PATCH 3/4] fix(control): backtick a mixed-case word flagged by clippy doc_markdown CI (gate / full-gate-build) caught it: -D warnings promotes clippy::doc_markdown to an error, and an un-backticked "PascalCase" in a register.rs test doc comment failed the workspace clippy pass. --- crates/dictyon/src/control/register.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/dictyon/src/control/register.rs b/crates/dictyon/src/control/register.rs index d262377..a4edc2b 100644 --- a/crates/dictyon/src/control/register.rs +++ b/crates/dictyon/src/control/register.rs @@ -158,7 +158,7 @@ mod tests { } /// WHY(#66): the empty-string `AuthURL` case is the exact shape a - /// PascalCase Go server sends alongside `MachineAuthorized: true` -- + /// `PascalCase` Go server sends alongside `MachineAuthorized: true` -- /// the field is present because the struct has no `omitempty` tag, not /// because interactive auth is actually needed. It must classify /// identically to the field being absent. From f4ea5ab95479ee94995cbf71abea0f6a1a9419a1 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 21:25:20 -0500 Subject: [PATCH 4/4] fix(control): address review on #66 register-outcome exhaustiveness Restores the client.poll_registration() call the report_register_outcome refactor silently dropped from examples/connect.rs, so the NeedsAuth branch performs the interactive-auth round trip again instead of only logging the URL. Retags four WHY(#66): doc comments to plain WHY: -- TAG(#NNN) is reserved for TODO/FIXME in the closed comment-tag set. --- crates/dictyon/examples/connect.rs | 23 +++++++++++++++++------ crates/dictyon/src/control/register.rs | 6 +++--- crates/dictyon/tests/register_flow/mod.rs | 2 +- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/crates/dictyon/examples/connect.rs b/crates/dictyon/examples/connect.rs index 08822a4..87cbe4b 100644 --- a/crates/dictyon/examples/connect.rs +++ b/crates/dictyon/examples/connect.rs @@ -134,16 +134,27 @@ async fn register_node( auth_key: Option<&str>, ) -> Result<(), ExampleError> { info!("registering…"); - report_register_outcome(client.register(stream, auth_key).await?); + match client.register(stream, auth_key).await? { + RegisterOutcome::NeedsAuth(url) => { + info!("visit to authorize: {url}"); + let followup = client.poll_registration(stream, url.as_str()).await?; + report_register_outcome(followup); + } + outcome => report_register_outcome(outcome), + } Ok(()) } -/// Log what a [`RegisterOutcome`] means for this example run. +/// Log what a terminal [`RegisterOutcome`] means for this example run. /// -/// A real caller (not this example) would poll again after `NeedsAuth` -/// once the user has visited the URL, and would retry registration with a -/// fresh node key after `RotateNodeKey`; this example only demonstrates -/// that every outcome is a distinguishable, handled case. +/// [`RegisterOutcome::NeedsAuth`] is handled by the caller (`register_node`) +/// before it reaches here: a real caller polls again via +/// [`ControlClient::poll_registration`] once the user has visited the URL, +/// so this example performs that round trip rather than only logging the +/// URL. A [`RegisterOutcome::NeedsAuth`] can still arrive here as the +/// *followup* poll's own result (the server has not observed the user +/// complete auth yet); a real caller would retry registration with a fresh +/// node key after `RotateNodeKey`. fn report_register_outcome(outcome: RegisterOutcome) { match outcome { RegisterOutcome::Authorized(resp) => { diff --git a/crates/dictyon/src/control/register.rs b/crates/dictyon/src/control/register.rs index a4edc2b..737dc93 100644 --- a/crates/dictyon/src/control/register.rs +++ b/crates/dictyon/src/control/register.rs @@ -157,7 +157,7 @@ mod tests { ); } - /// WHY(#66): the empty-string `AuthURL` case is the exact shape a + /// WHY: the empty-string `AuthURL` case is the exact shape a /// `PascalCase` Go server sends alongside `MachineAuthorized: true` -- /// the field is present because the struct has no `omitempty` tag, not /// because interactive auth is actually needed. It must classify @@ -196,7 +196,7 @@ mod tests { } } - /// WHY(#66): this used to discard the rejection and fall into the + /// WHY: this used to discard the rejection and fall into the /// empty-URL auth flow. `Error` must win over `MachineAuthorized` and /// `AuthURL` unconditionally. #[test] @@ -287,7 +287,7 @@ mod tests { ); } - /// WHY(#66): this used to become `Authorized` by inferring success from + /// WHY: this used to become `Authorized` by inferring success from /// an absent `AuthURL` alone, discarding `MachineAuthorized: false`. #[test] fn unauthorized_with_no_signal_is_contradictory_not_authorized() { diff --git a/crates/dictyon/tests/register_flow/mod.rs b/crates/dictyon/tests/register_flow/mod.rs index 2f5ecac..c172c05 100644 --- a/crates/dictyon/tests/register_flow/mod.rs +++ b/crates/dictyon/tests/register_flow/mod.rs @@ -5,7 +5,7 @@ use super::*; -/// WHY(#66): `poll_registration` runs the same wire round-trip as +/// WHY: `poll_registration` runs the same wire round-trip as /// `register` and must validate its response through the same exhaustive /// classifier -- an unvalidated followup response was the original defect's /// second, untested entry point. This drives `register` to `NeedsAuth` and