From 4106fc4bc4edd752f098aaf59b3b542091892a4c Mon Sep 17 00:00:00 2001 From: Mike Swanson Date: Tue, 2 Jun 2026 10:48:51 -0700 Subject: [PATCH] style(enroll): cargo fmt --all (satisfy CI fmt gate) The Phase A work passed cargo check + clippy + tests locally but missed `cargo fmt --all -- --check` (the first step of the Linux CI job): module ordering in db/mod.rs and two trailing-comment alignments in rate_limit.rs. No logic change. Agent build failure on the prior run was transient infra (verified: agent crate compiles clean locally). Co-Authored-By: Claude Opus 4.8 (1M context) --- server/src/api/enroll.rs | 499 ++++++++++++++++------------ server/src/auth/enrollment_keys.rs | 5 +- server/src/db/enrollment_keys.rs | 11 +- server/src/db/mod.rs | 2 +- server/src/middleware/rate_limit.rs | 4 +- 5 files changed, 296 insertions(+), 225 deletions(-) diff --git a/server/src/api/enroll.rs b/server/src/api/enroll.rs index b0cf69f..2aaa1eb 100644 --- a/server/src/api/enroll.rs +++ b/server/src/api/enroll.rs @@ -218,11 +218,7 @@ fn is_collision(existing: &db::machines::Machine, incoming_hostname: &str) -> bo /// Mint a `cak_`, store its hash bound to `machine_id` + tenant, and return the /// plaintext. Shared by the new/reuse/move active paths. -async fn mint_cak( - db: &db::Database, - machine_id: Uuid, - tenant_id: Uuid, -) -> ApiResult { +async fn mint_cak(db: &db::Database, machine_id: Uuid, tenant_id: Uuid) -> ApiResult { let plaintext = agent_keys::generate_agent_key(); let key_hash = agent_keys::hash_agent_key(&plaintext); db::agent_keys::insert_agent_key(db.pool(), machine_id, &key_hash, Some(tenant_id)) @@ -287,11 +283,16 @@ pub async fn enroll( // site_code is not distinguishable by timing (enumeration oracle). equalize_reject_timing(&req.enrollment_key); state.rate_limits.enroll.record_failure(site_code, ip); - audit(db, db::events::EventTypes::ENROLL_REJECTED, ip, json!({ - "reason": "unknown_site_code", - "site_code": site_code, - "machine_uid": machine_uid, - })) + audit( + db, + db::events::EventTypes::ENROLL_REJECTED, + ip, + json!({ + "reason": "unknown_site_code", + "site_code": site_code, + "machine_uid": machine_uid, + }), + ) .await; tracing::warn!("[ENROLL] unknown site_code={} from {}", site_code, ip); // Same opaque rejection shape AND the same KDF cost as a bad key — do not @@ -322,13 +323,21 @@ pub async fn enroll( // has no active key" is not distinguishable by timing from a bad key. equalize_reject_timing(&req.enrollment_key); state.rate_limits.enroll.record_failure(site_code, ip); - audit(db, db::events::EventTypes::ENROLL_REJECTED, ip, json!({ - "reason": "no_active_key", - "site_code": site_code, - "machine_uid": machine_uid, - })) + audit( + db, + db::events::EventTypes::ENROLL_REJECTED, + ip, + json!({ + "reason": "no_active_key", + "site_code": site_code, + "machine_uid": machine_uid, + }), + ) .await; - tracing::warn!("[ENROLL] no active enrollment key for site_code={}", site_code); + tracing::warn!( + "[ENROLL] no active enrollment key for site_code={}", + site_code + ); return Err(ApiError::new( StatusCode::UNAUTHORIZED, "ENROLL_REJECTED", @@ -347,13 +356,22 @@ pub async fn enroll( if !enrollment_keys::verify_enrollment_key(&req.enrollment_key, &active_key.key_hash) { state.rate_limits.enroll.record_failure(site_code, ip); - audit(db, db::events::EventTypes::ENROLL_REJECTED, ip, json!({ - "reason": "bad_enrollment_key", - "site_code": site_code, - "machine_uid": machine_uid, - })) + audit( + db, + db::events::EventTypes::ENROLL_REJECTED, + ip, + json!({ + "reason": "bad_enrollment_key", + "site_code": site_code, + "machine_uid": machine_uid, + }), + ) .await; - tracing::warn!("[ENROLL] bad enrollment key for site_code={} from {}", site_code, ip); + tracing::warn!( + "[ENROLL] bad enrollment key for site_code={} from {}", + site_code, + ip + ); return Err(ApiError::new( StatusCode::UNAUTHORIZED, "ENROLL_REJECTED", @@ -366,7 +384,12 @@ pub async fn enroll( // Build the label/identity params shared by the create/update paths. let tags = effective_tags(&req.labels); - let company = req.labels.company.as_deref().map(str::trim).filter(|s| !s.is_empty()); + let company = req + .labels + .company + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()); let site_label = req .labels .site @@ -386,25 +409,137 @@ pub async fn enroll( let mut attempts = 0u8; loop { attempts += 1; - let existing = - match db::machines::get_machine_by_tenant_uid(db.pool(), tenant_id, machine_uid).await { - Ok(e) => e, - Err(e) => { - tracing::error!("[ENROLL] DB error on dedup lookup: {}", e); - return Err(ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - "INTERNAL_ERROR", - "Internal server error", - )); - } - }; + let existing = match db::machines::get_machine_by_tenant_uid( + db.pool(), + tenant_id, + machine_uid, + ) + .await + { + Ok(e) => e, + Err(e) => { + tracing::error!("[ENROLL] DB error on dedup lookup: {}", e); + return Err(ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "INTERNAL_ERROR", + "Internal server error", + )); + } + }; match existing { - // -- Reuse / collision path ------------------------------------------- - Some(existing) => { - // Collision gate: a seemingly-different live endpoint resolving to an - // existing uid -> pending, alert, NO usable cak_ minted. - if is_collision(&existing, hostname) { + // -- Reuse / collision path ------------------------------------------- + Some(existing) => { + // Collision gate: a seemingly-different live endpoint resolving to an + // existing uid -> pending, alert, NO usable cak_ minted. + if is_collision(&existing, hostname) { + let params = enroll_params( + &existing.agent_id, + hostname, + machine_uid, + tenant_id, + site.id, + company, + site_label, + &tags, + "pending", + ); + let machine = + db::machines::update_enrolled_machine(db.pool(), existing.id, ¶ms) + .await + .map_err(map_update_err)?; + + audit( + db, + db::events::EventTypes::ENROLL_COLLISION_PENDING, + ip, + json!({ + "machine_id": machine.id, + "machine_uid": machine_uid, + "site_code": site_code, + "existing_hostname": existing.hostname, + "incoming_hostname": hostname, + "heuristic": "online_existing_with_different_hostname (PROVISIONAL)", + }), + ) + .await; + // TODO(SPEC-016): wire to #dev-alerts — collision requires operator + // confirmation in the dashboard before this endpoint may activate. + tracing::warn!( + "[ENROLL] machine_uid collision -> PENDING: machine_id={} site_code={} \ + existing_host={} incoming_host={} from {}", + machine.id, + site_code, + existing.hostname, + hostname, + ip + ); + + return Ok(( + StatusCode::ACCEPTED, + Json(EnrollResponse { + machine_id: machine.id, + key: None, + enrollment_state: "pending".to_string(), + disposition: "collision_pending".to_string(), + }), + )); + } + + // Cross-site guard (SPEC-016 Phase A, HIGH): if this machine_uid is + // already bound to a DIFFERENT site, REFUSE — do not move it and do not + // mint a key. `machine_uid` is a raw, spoofable client string in Phase A, + // so a silent move here is a hijack vector: a holder of site B's key could + // claim a site-A machine merely by presenting its uid, repointing the row + // and minting a fresh `cak_` on site B. Deliberate moves are out of scope + // for Phase A and arrive with the Phase-B `--reassign` flow + dashboard + // (SPEC-016 §"Explicitly out of scope"); until then this is the explicit + // accidental-move / hijack guard the spec calls for. + // + // Only `Some(other_site)` is a conflict. `None` (a legacy / connect-path / + // support-code row that never had a relational site binding) is treated as + // a first-time bind to the enrolling site, NOT a cross-site move — there is + // no prior owning site to hijack from. + if let Some(existing_site) = existing.site_id { + if existing_site != site.id { + audit( + db, + db::events::EventTypes::ENROLL_SITE_CONFLICT, + ip, + json!({ + "machine_id": existing.id, + "machine_uid": machine_uid, + // Record the bound site id for the operator audit trail; the + // response body below leaks nothing about the other site. + "bound_site_id": existing_site, + "attempted_site_code": site_code, + "attempted_site_id": site.id, + }), + ) + .await; + // TODO(SPEC-016): wire to #dev-alerts — cross-site enroll refused + // (possible accidental move or a cross-site claim attempt). + tracing::warn!( + "[ENROLL] cross-site conflict REFUSED: machine_id={} already bound to \ + a different site; attempted site_code={} from {}", + existing.id, + site_code, + ip + ); + // Opaque-enough: states the machine is already enrolled elsewhere + // and how to move it deliberately, without naming the other site. + return Err(ApiError::new( + StatusCode::CONFLICT, + "ENROLL_SITE_CONFLICT", + "This machine is already enrolled at another site. A deliberate \ + move requires the operator-initiated reassignment flow.", + )); + } + } + + // Same-site reuse (re-image / re-install) or first relational bind of a + // legacy/connect-path row (existing.site_id was NULL). No site move occurs + // on the unauthenticated path in Phase A. let params = enroll_params( &existing.agent_id, hostname, @@ -414,195 +549,123 @@ pub async fn enroll( company, site_label, &tags, - "pending", + "active", ); - let machine = db::machines::update_enrolled_machine(db.pool(), existing.id, ¶ms) - .await - .map_err(map_update_err)?; + let machine = + db::machines::update_enrolled_machine(db.pool(), existing.id, ¶ms) + .await + .map_err(map_update_err)?; - audit(db, db::events::EventTypes::ENROLL_COLLISION_PENDING, ip, json!({ - "machine_id": machine.id, - "machine_uid": machine_uid, - "site_code": site_code, - "existing_hostname": existing.hostname, - "incoming_hostname": hostname, - "heuristic": "online_existing_with_different_hostname (PROVISIONAL)", - })) + let cak = mint_cak(db, machine.id, tenant_id).await?; + + audit( + db, + db::events::EventTypes::ENROLL_REUSE, + ip, + json!({ + "machine_id": machine.id, + "machine_uid": machine_uid, + "site_code": site_code, + }), + ) .await; - // TODO(SPEC-016): wire to #dev-alerts — collision requires operator - // confirmation in the dashboard before this endpoint may activate. - tracing::warn!( - "[ENROLL] machine_uid collision -> PENDING: machine_id={} site_code={} \ - existing_host={} incoming_host={} from {}", - machine.id, site_code, existing.hostname, hostname, ip + tracing::info!( + "[ENROLL] reuse: machine_id={} re-enrolled at site_code={} from {}", + machine.id, + site_code, + ip ); return Ok(( - StatusCode::ACCEPTED, + StatusCode::OK, Json(EnrollResponse { machine_id: machine.id, - key: None, - enrollment_state: "pending".to_string(), - disposition: "collision_pending".to_string(), + key: Some(cak), + enrollment_state: "active".to_string(), + disposition: "reuse".to_string(), }), )); } - // Cross-site guard (SPEC-016 Phase A, HIGH): if this machine_uid is - // already bound to a DIFFERENT site, REFUSE — do not move it and do not - // mint a key. `machine_uid` is a raw, spoofable client string in Phase A, - // so a silent move here is a hijack vector: a holder of site B's key could - // claim a site-A machine merely by presenting its uid, repointing the row - // and minting a fresh `cak_` on site B. Deliberate moves are out of scope - // for Phase A and arrive with the Phase-B `--reassign` flow + dashboard - // (SPEC-016 §"Explicitly out of scope"); until then this is the explicit - // accidental-move / hijack guard the spec calls for. - // - // Only `Some(other_site)` is a conflict. `None` (a legacy / connect-path / - // support-code row that never had a relational site binding) is treated as - // a first-time bind to the enrolling site, NOT a cross-site move — there is - // no prior owning site to hijack from. - if let Some(existing_site) = existing.site_id { - if existing_site != site.id { - audit(db, db::events::EventTypes::ENROLL_SITE_CONFLICT, ip, json!({ - "machine_id": existing.id, - "machine_uid": machine_uid, - // Record the bound site id for the operator audit trail; the - // response body below leaks nothing about the other site. - "bound_site_id": existing_site, - "attempted_site_code": site_code, - "attempted_site_id": site.id, - })) - .await; - // TODO(SPEC-016): wire to #dev-alerts — cross-site enroll refused - // (possible accidental move or a cross-site claim attempt). - tracing::warn!( - "[ENROLL] cross-site conflict REFUSED: machine_id={} already bound to \ - a different site; attempted site_code={} from {}", - existing.id, site_code, ip - ); - // Opaque-enough: states the machine is already enrolled elsewhere - // and how to move it deliberately, without naming the other site. - return Err(ApiError::new( - StatusCode::CONFLICT, - "ENROLL_SITE_CONFLICT", - "This machine is already enrolled at another site. A deliberate \ - move requires the operator-initiated reassignment flow.", - )); - } - } - - // Same-site reuse (re-image / re-install) or first relational bind of a - // legacy/connect-path row (existing.site_id was NULL). No site move occurs - // on the unauthenticated path in Phase A. - let params = enroll_params( - &existing.agent_id, - hostname, - machine_uid, - tenant_id, - site.id, - company, - site_label, - &tags, - "active", - ); - let machine = db::machines::update_enrolled_machine(db.pool(), existing.id, ¶ms) - .await - .map_err(map_update_err)?; - - let cak = mint_cak(db, machine.id, tenant_id).await?; - - audit(db, db::events::EventTypes::ENROLL_REUSE, ip, json!({ - "machine_id": machine.id, - "machine_uid": machine_uid, - "site_code": site_code, - })) - .await; - tracing::info!( - "[ENROLL] reuse: machine_id={} re-enrolled at site_code={} from {}", - machine.id, site_code, ip - ); - - return Ok(( - StatusCode::OK, - Json(EnrollResponse { - machine_id: machine.id, - key: Some(cak), - enrollment_state: "active".to_string(), - disposition: "reuse".to_string(), - }), - )); - } - - // -- New enrollment ---------------------------------------------------- - None => { - // Fresh opaque agent_id for the new row's `agent_id UNIQUE` column. The - // agent's own config-UUID story is Phase B; the server only needs a - // unique non-null value here, and the authoritative identity is the - // minted cak_ -> machine binding. - let agent_id = format!("enroll-{}", Uuid::new_v4()); - let params = enroll_params( - &agent_id, - hostname, - machine_uid, - tenant_id, - site.id, - company, - site_label, - &tags, - "active", - ); - let machine = match db::machines::insert_enrolled_machine(db.pool(), ¶ms).await { - Ok(m) => m, - // TOCTOU loser: a concurrent first-enroll of the same machine_uid won - // the race and committed its row between our lookup and this INSERT, so - // the partial unique index on `machine_uid` rejects ours. Loop back: the - // re-lookup now finds the winner's row and we converge to reuse instead - // of 500ing. Capped so a non-uid unique violation (or any persistent - // conflict) surfaces as a 500 rather than spinning. - Err(e) if is_machine_uid_conflict(&e) && attempts < 2 => { - tracing::info!( - "[ENROLL] concurrent first-enroll race on machine_uid; \ + // -- New enrollment ---------------------------------------------------- + None => { + // Fresh opaque agent_id for the new row's `agent_id UNIQUE` column. The + // agent's own config-UUID story is Phase B; the server only needs a + // unique non-null value here, and the authoritative identity is the + // minted cak_ -> machine binding. + let agent_id = format!("enroll-{}", Uuid::new_v4()); + let params = enroll_params( + &agent_id, + hostname, + machine_uid, + tenant_id, + site.id, + company, + site_label, + &tags, + "active", + ); + let machine = match db::machines::insert_enrolled_machine(db.pool(), ¶ms).await + { + Ok(m) => m, + // TOCTOU loser: a concurrent first-enroll of the same machine_uid won + // the race and committed its row between our lookup and this INSERT, so + // the partial unique index on `machine_uid` rejects ours. Loop back: the + // re-lookup now finds the winner's row and we converge to reuse instead + // of 500ing. Capped so a non-uid unique violation (or any persistent + // conflict) surfaces as a 500 rather than spinning. + Err(e) if is_machine_uid_conflict(&e) && attempts < 2 => { + tracing::info!( + "[ENROLL] concurrent first-enroll race on machine_uid; \ retrying as reuse (site_code={} from {})", - site_code, ip - ); - continue; - } - Err(e) => { - tracing::error!("[ENROLL] DB error inserting enrolled machine: {}", e); - return Err(ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - "INTERNAL_ERROR", - "Failed to register machine", - )); - } - }; + site_code, + ip + ); + continue; + } + Err(e) => { + tracing::error!("[ENROLL] DB error inserting enrolled machine: {}", e); + return Err(ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "INTERNAL_ERROR", + "Failed to register machine", + )); + } + }; - let cak = mint_cak(db, machine.id, tenant_id).await?; + let cak = mint_cak(db, machine.id, tenant_id).await?; - audit(db, db::events::EventTypes::ENROLL_NEW, ip, json!({ - "machine_id": machine.id, - "machine_uid": machine_uid, - "site_code": site_code, - "hostname": hostname, - })) - .await; - // TODO(SPEC-016): wire to #dev-alerts — new-enrollment tripwire. - tracing::info!( - "[ENROLL] new: machine_id={} hostname={} site_code={} from {}", - machine.id, hostname, site_code, ip - ); + audit( + db, + db::events::EventTypes::ENROLL_NEW, + ip, + json!({ + "machine_id": machine.id, + "machine_uid": machine_uid, + "site_code": site_code, + "hostname": hostname, + }), + ) + .await; + // TODO(SPEC-016): wire to #dev-alerts — new-enrollment tripwire. + tracing::info!( + "[ENROLL] new: machine_id={} hostname={} site_code={} from {}", + machine.id, + hostname, + site_code, + ip + ); - return Ok(( - StatusCode::CREATED, - Json(EnrollResponse { - machine_id: machine.id, - key: Some(cak), - enrollment_state: "active".to_string(), - disposition: "new".to_string(), - }), - )); - } + return Ok(( + StatusCode::CREATED, + Json(EnrollResponse { + machine_id: machine.id, + key: Some(cak), + enrollment_state: "active".to_string(), + disposition: "new".to_string(), + }), + )); + } } } } @@ -636,7 +699,12 @@ fn effective_tags(labels: &EnrollLabels) -> Vec { .map(|t| t.trim().to_string()) .filter(|t| !t.is_empty()) .collect(); - if let Some(d) = labels.department.as_deref().map(str::trim).filter(|s| !s.is_empty()) { + if let Some(d) = labels + .department + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + { tags.push(format!("department:{}", d)); } if let Some(d) = labels @@ -708,7 +776,8 @@ mod tests { fn timing_equalizer_phc_is_a_valid_parseable_hash() { // A wrong password must verify to `false` WITHOUT erroring — proving the PHC // parsed and the KDF actually ran (a parse failure would surface as `Err`). - let res = crate::auth::password::verify_password("cek_anything_at_all", TIMING_EQUALIZER_PHC); + let res = + crate::auth::password::verify_password("cek_anything_at_all", TIMING_EQUALIZER_PHC); assert!( res.is_ok(), "TIMING_EQUALIZER_PHC must be a valid PHC string so the KDF runs; got Err: {:?}", diff --git a/server/src/auth/enrollment_keys.rs b/server/src/auth/enrollment_keys.rs index 2b63b38..e32e2d0 100644 --- a/server/src/auth/enrollment_keys.rs +++ b/server/src/auth/enrollment_keys.rs @@ -178,7 +178,10 @@ mod tests { #[test] fn fingerprint_differs_per_key() { - assert_ne!(compute_fingerprint("cek_aaa"), compute_fingerprint("cek_bbb")); + assert_ne!( + compute_fingerprint("cek_aaa"), + compute_fingerprint("cek_bbb") + ); } #[test] diff --git a/server/src/db/enrollment_keys.rs b/server/src/db/enrollment_keys.rs index 96c27bd..82715f8 100644 --- a/server/src/db/enrollment_keys.rs +++ b/server/src/db/enrollment_keys.rs @@ -102,12 +102,11 @@ pub async fn rotate_key( let mut tx = pool.begin().await?; // Highest existing version for this site (NULL -> 0 so the first key is v1). - let current_max: Option = sqlx::query_scalar( - "SELECT MAX(version) FROM site_enrollment_keys WHERE site_id = $1", - ) - .bind(site_id) - .fetch_one(&mut *tx) - .await?; + let current_max: Option = + sqlx::query_scalar("SELECT MAX(version) FROM site_enrollment_keys WHERE site_id = $1") + .bind(site_id) + .fetch_one(&mut *tx) + .await?; let next_version = current_max.unwrap_or(0) + 1; // Deactivate the current active key (if any), stamping rotated_at. diff --git a/server/src/db/mod.rs b/server/src/db/mod.rs index 97afb9e..a4432a3 100644 --- a/server/src/db/mod.rs +++ b/server/src/db/mod.rs @@ -7,9 +7,9 @@ pub mod agent_keys; pub mod enrollment_keys; pub mod events; pub mod machines; -pub mod sites; pub mod releases; pub mod sessions; +pub mod sites; pub mod support_codes; pub mod tenancy; pub mod users; diff --git a/server/src/middleware/rate_limit.rs b/server/src/middleware/rate_limit.rs index 72f21e7..eeb420e 100644 --- a/server/src/middleware/rate_limit.rs +++ b/server/src/middleware/rate_limit.rs @@ -708,7 +708,7 @@ mod tests { let t0 = Instant::now(); assert!(lim.check_at("SITE-A", ip(1), t0)); assert!(!lim.check_at("SITE-A", ip(1), t0)); // same key over cap - // Different site, same IP -> independent bucket. + // Different site, same IP -> independent bucket. assert!(lim.check_at("SITE-B", ip(1), t0)); // Same site, different IP -> independent bucket. assert!(lim.check_at("SITE-A", ip(2), t0)); @@ -723,7 +723,7 @@ mod tests { // Not yet tripped: a check still admits. assert!(lim.check_at("SITE-A", ip(1), t0)); lim.record_failure_at("SITE-A", ip(1), t0); // 3rd -> trips - // Now locked out: check denies even though under the request cap. + // Now locked out: check denies even though under the request cap. assert!(!lim.check_at("SITE-A", ip(1), t0)); }