diff --git a/crates/agent-channel-client/INTENT.md b/crates/agent-channel-client/INTENT.md index b7a3b1cf4..675268198 100644 --- a/crates/agent-channel-client/INTENT.md +++ b/crates/agent-channel-client/INTENT.md @@ -9,6 +9,7 @@ This crate is the agent's side of the channel defined in `agent-channel-proto`. - The backoff restarts from 1 s only after a stream has stayed authenticated for 60 s, so that a stream that authenticates and then drops at once keeps backing off. After such a long-lived stream closes, the first reconnect waits a delay drawn uniformly from `[0, 30 s]`, so that an authority restart doesn't bring every agent back within the same second. - A server message that cannot be decoded, or whose `id` is not a UUID, closes the stream as a transient failure without a reply, so the agent never echoes an unchecked `id` and never sends an envelope larger than `CONTRACT.agent-identity.md` §7.1 allows. +- Text from the authority, a status message or the reason of a push, is logged with control characters, bidirectional overrides and backslashes escaped and is cut to 256 characters, and an `error-code` that is not a lowercase snake_case code of at most 64 bytes is logged as invalid, so that an authority cannot forge log lines or flood the log. ## Handshake diff --git a/crates/agent-channel-client/src/closure.rs b/crates/agent-channel-client/src/closure.rs index 409193a2a..f1b43d9d0 100644 --- a/crates/agent-channel-client/src/closure.rs +++ b/crates/agent-channel-client/src/closure.rs @@ -7,6 +7,9 @@ use std::error::Error; use std::fmt; +use tonic::Code; + +use crate::loggable::loggable; use crate::transport::NotGrpc; /// Why a stream closed or its opening failed ([CONTRACT.agent-identity.md §7.4 "Termination"][contract]). @@ -16,15 +19,54 @@ use crate::transport::NotGrpc; pub struct Closure { pub class: ClosureClass, /// The gRPC status the authority closed the stream with, if it sent one. - pub grpc_code: Option, + pub grpc_code: Option, /// The `error-code` trailer. - pub error_code: Option, + pub error_code: Option, /// The `server-time` trailer. pub server_time: Option, /// A description of the cause, for logs. + /// + /// The message of a status the authority sent is escaped and truncated so that it is safe to log. pub reason: String, } +/// The `error-code` trailer of a status. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ErrorCode { + /// A value in the form of an error code: snake_case ASCII ([CONTRACT.agent-identity.md §1 "Protocol + /// conventions"][contract]), starting with a letter, of at most 64 bytes, and not starting with the enrollment token + /// prefix `dvaet1`. + /// + /// [contract]: ../../../docs/agent-identity/CONTRACT.agent-identity.md#1-protocol-conventions + Code(String), + /// Any other value, which is not kept so that its text never reaches a log. + Invalid, +} + +impl ErrorCode { + fn parse(value: &str) -> Self { + if value.len() <= 64 + && value.as_bytes().first().is_some_and(u8::is_ascii_lowercase) + && value + .bytes() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'_') + && !value.starts_with("dvaet1") + { + Self::Code(value.to_owned()) + } else { + Self::Invalid + } + } + + /// Returns the code, or `` for a value that is not one. + pub fn as_str(&self) -> &str { + match self { + Self::Code(code) => code, + Self::Invalid => "", + } + } +} + /// What a closure means for the identity ([CONTRACT.agent-identity.md §5.4 "Errors"][contract]). /// /// [contract]: ../../../docs/agent-identity/CONTRACT.agent-identity.md#54-errors @@ -81,8 +123,9 @@ impl Closure { /// Classifies the status a stream ended with. /// - /// A status the authority sent is classified by its `error-code` trailer; a status the client derived from a - /// connection failure carries the failure as its source. + /// A status the authority sent is classified by its gRPC code with its `error-code` trailer, as exact pairs, and + /// any other pair is transient; a status the client derived from a connection failure carries the failure as its + /// source. pub(crate) fn from_status(status: &tonic::Status) -> Self { let mut cause = status.source(); while let Some(error) = cause { @@ -94,26 +137,25 @@ impl Closure { if let Some(source) = status.source() { return Self::outage(error_chain(status.message(), source)); } - let trailer = |name: &str| { - status - .metadata() - .get(name) - .and_then(|value| value.to_str().ok()) - .map(str::to_owned) - }; - let error_code = trailer("error-code"); - let class = match error_code.as_deref() { - Some("identity_revoked") => ClosureClass::Terminal(TerminalCode::IdentityRevoked), - Some("identity_unknown") => ClosureClass::Terminal(TerminalCode::IdentityUnknown), - Some("certificate_expired") => ClosureClass::CertificateExpired, + let metadata = status.metadata(); + let error_code = metadata + .get("error-code") + .map(|value| value.to_str().map_or(ErrorCode::Invalid, ErrorCode::parse)); + let class = match (status.code(), error_code.as_ref().map(ErrorCode::as_str)) { + (Code::PermissionDenied, Some("identity_revoked")) => ClosureClass::Terminal(TerminalCode::IdentityRevoked), + (Code::Unauthenticated, Some("identity_unknown")) => ClosureClass::Terminal(TerminalCode::IdentityUnknown), + (Code::Unauthenticated, Some("certificate_expired")) => ClosureClass::CertificateExpired, _ => ClosureClass::Transient(TransientKind::Outage), }; Self { class, grpc_code: Some(status.code()), error_code, - server_time: trailer("server-time"), - reason: status.message().to_owned(), + server_time: metadata + .get("server-time") + .and_then(|value| value.to_str().ok()) + .map(str::to_owned), + reason: loggable(status.message()), } } } @@ -129,3 +171,106 @@ pub(crate) fn error_chain(message: &str, source: &(dyn Error + 'static)) -> Stri } chain } + +#[cfg(test)] +mod tests { + use rstest::rstest; + use tonic::Status; + use tonic::metadata::{MetadataMap, MetadataValue}; + + use super::*; + + /// A status as the authority sends it, with an `error-code` trailer. + fn status(code: Code, error_code: &str) -> Status { + let mut metadata = MetadataMap::new(); + metadata.insert("error-code", error_code.parse().expect("ASCII error code")); + Status::with_metadata(code, "closed by the test", metadata) + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §channel-termination + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §errors + #[rstest] + #[case::revoked( + Code::PermissionDenied, + "identity_revoked", + ClosureClass::Terminal(TerminalCode::IdentityRevoked) + )] + #[case::unknown( + Code::Unauthenticated, + "identity_unknown", + ClosureClass::Terminal(TerminalCode::IdentityUnknown) + )] + #[case::expired(Code::Unauthenticated, "certificate_expired", ClosureClass::CertificateExpired)] + fn classifies_each_status_and_error_code_pair_of_the_contract( + #[case] code: Code, + #[case] error_code: &str, + #[case] class: ClosureClass, + ) { + assert_eq!(Closure::from_status(&status(code, error_code)).class, class); + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §channel-termination + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §errors + #[rstest] + #[case::revoked_unauthenticated(Code::Unauthenticated, "identity_revoked")] + #[case::revoked_ok(Code::Ok, "identity_revoked")] + #[case::revoked_unavailable(Code::Unavailable, "identity_revoked")] + #[case::unknown_permission_denied(Code::PermissionDenied, "identity_unknown")] + #[case::unknown_ok(Code::Ok, "identity_unknown")] + #[case::expired_permission_denied(Code::PermissionDenied, "certificate_expired")] + #[case::expired_ok(Code::Ok, "certificate_expired")] + #[case::expired_internal(Code::Internal, "certificate_expired")] + fn a_terminal_or_expired_code_with_another_status_is_an_outage(#[case] code: Code, #[case] error_code: &str) { + assert_eq!( + Closure::from_status(&status(code, error_code)).class, + ClosureClass::Transient(TransientKind::Outage) + ); + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §conventions + #[rstest] + #[case::single_letter("a")] + #[case::with_digits("code2_v3")] + #[case::future_code("future_code")] + // oracle: intent crates/agent-channel-client/INTENT.md §Connection + #[case::at_64_bytes("a".repeat(64))] + fn keeps_an_error_code_in_snake_case(#[case] error_code: String) { + let closure = Closure::from_status(&status(Code::Unauthenticated, &error_code)); + assert_eq!(closure.error_code, Some(ErrorCode::Code(error_code))); + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §conventions + #[rstest] + #[case::empty("")] + #[case::upper_case("Identity_Revoked")] + #[case::digit_first("2fa")] + #[case::underscore_first("_code")] + #[case::hyphen("identity-revoked")] + #[case::space("identity revoked")] + // oracle: intent crates/agent-channel-client/INTENT.md §Connection + #[case::past_64_bytes("a".repeat(65))] + // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Security invariants + #[case::token_prefix("dvaet1_secret")] + fn does_not_keep_an_invalid_error_code(#[case] error_code: String) { + let closure = Closure::from_status(&status(Code::Unauthenticated, &error_code)); + assert_eq!(closure.error_code, Some(ErrorCode::Invalid)); + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §conventions + #[test] + fn does_not_keep_an_error_code_that_is_not_visible_ascii() { + let mut metadata = MetadataMap::new(); + let value = MetadataValue::try_from(b"identity_revoked\xe9".as_slice()).expect("opaque header value"); + metadata.insert("error-code", value); + let closure = Closure::from_status(&Status::with_metadata(Code::PermissionDenied, "", metadata)); + assert_eq!(closure.error_code, Some(ErrorCode::Invalid)); + assert_eq!(closure.class, ClosureClass::Transient(TransientKind::Outage)); + } + + // oracle: intent crates/agent-channel-client/INTENT.md §Connection + #[test] + fn escapes_the_message_of_a_status_the_authority_sent() { + let closure = Closure::from_status(&Status::new(Code::Unavailable, "down\r\nINFO forged")); + assert_eq!(closure.reason, r"down\r\nINFO forged"); + } +} diff --git a/crates/agent-channel-client/src/lib.rs b/crates/agent-channel-client/src/lib.rs index 751bc1bd1..5cf6cffef 100644 --- a/crates/agent-channel-client/src/lib.rs +++ b/crates/agent-channel-client/src/lib.rs @@ -20,11 +20,12 @@ use crate::transport::Transport; mod backoff; mod closure; +mod loggable; mod stream; mod transport; pub use crate::backoff::{Backoff, uniform_jitter, uniform_spread}; -pub use crate::closure::{Closure, ClosureClass, TerminalCode, TransientKind}; +pub use crate::closure::{Closure, ClosureClass, ErrorCode, TerminalCode, TransientKind}; const KEEPALIVE_INTERVAL: Duration = Duration::from_secs(30); const OPENING_TIMEOUT: Duration = Duration::from_secs(30); @@ -90,8 +91,12 @@ pub enum Event { /// The authority answered the `Hello` with `Welcome`: the stream is authenticated. Authenticated, /// The authority asks the identity to renew; the caller acknowledges `id`. + /// + /// The authority's `reason` is escaped and truncated so that it is safe to log. RenewRequested { id: String, reason: String }, /// The authority asks for a new stream, which the caller opens before dropping this channel. + /// + /// The authority's `reason` is escaped and truncated so that it is safe to log. Reconnect { id: String, reason: String }, /// A newer effective config, as a JSON object; the caller acknowledges `id`. ConfigUpdate { id: String, config_json: String }, diff --git a/crates/agent-channel-client/src/loggable.rs b/crates/agent-channel-client/src/loggable.rs new file mode 100644 index 000000000..6dfcfc9d2 --- /dev/null +++ b/crates/agent-channel-client/src/loggable.rs @@ -0,0 +1,64 @@ +//! Text the authority chose, made safe to log. + +/// The most characters [`loggable`] keeps, escapes included, before its truncation marker. +const MAX_LOGGED_CHARS: usize = 256; +/// The marker that ends a truncated text. +const TRUNCATED: &str = " [truncated]"; + +/// Returns `text` with each control character, backslash and other character that Rust's `Debug` escapes written as +/// that escape, cut before the first character that would take it past [`MAX_LOGGED_CHARS`] and then marked as +/// truncated. +/// +/// The authority's text can't then start a forged log line or grow a log line without bound. +pub(crate) fn loggable(text: &str) -> String { + let mut logged = String::new(); + let mut len = 0; + for character in text.chars() { + let start = logged.len(); + // Quotes need no escape inside a log field, and escaping them would only clutter the text. + if matches!(character, '"' | '\'') { + logged.push(character); + } else { + logged.extend(character.escape_debug()); + } + len += logged[start..].chars().count(); + if MAX_LOGGED_CHARS < len { + logged.truncate(start); + logged.push_str(TRUNCATED); + break; + } + } + logged +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + + use super::*; + + // oracle: intent crates/agent-channel-client/INTENT.md §Connection + #[rstest] + #[case::carriage_return_and_line_feed("a\r\nb", r"a\r\nb")] + #[case::escape("a\u{1b}[2Jb", r"a\u{1b}[2Jb")] + #[case::delete("a\u{7f}b", r"a\u{7f}b")] + #[case::c1_next_line("a\u{85}b", r"a\u{85}b")] + #[case::nul_and_tab("a\0\tb", r"a\0\tb")] + #[case::backslash(r"a\nb", r"a\\nb")] + #[case::right_to_left_override("a\u{202e}b", r"a\u{202e}b")] + #[case::printable_text_and_quotes("certificate \"rotated\" é", "certificate \"rotated\" é")] + fn escapes_each_control_character(#[case] text: &str, #[case] logged: &str) { + assert_eq!(loggable(text), logged); + } + + // oracle: intent crates/agent-channel-client/INTENT.md §Connection + #[rstest] + #[case::at_the_bound("a".repeat(256), "a".repeat(256))] + #[case::past_the_bound("a".repeat(257), format!("{} [truncated]", "a".repeat(256)))] + #[case::multi_byte_character_at_the_bound("é".repeat(257), format!("{} [truncated]", "é".repeat(256)))] + #[case::escape_across_the_bound(format!("{}\n", "a".repeat(255)), format!("{} [truncated]", "a".repeat(255)))] + #[case::quote_past_the_bound(format!("{}\"", "a".repeat(256)), format!("{} [truncated]", "a".repeat(256)))] + fn truncates_past_256_characters_without_splitting_one(#[case] text: String, #[case] logged: String) { + assert_eq!(loggable(&text), logged); + } +} diff --git a/crates/agent-channel-client/src/stream.rs b/crates/agent-channel-client/src/stream.rs index a7039908f..55bccce3f 100644 --- a/crates/agent-channel-client/src/stream.rs +++ b/crates/agent-channel-client/src/stream.rs @@ -19,7 +19,8 @@ use tower::Layer as _; use uuid::Uuid; use crate::backoff::BackoffState; -use crate::closure::{Closure, ClosureClass}; +use crate::closure::{Closure, ClosureClass, ErrorCode}; +use crate::loggable::loggable; use crate::transport::endpoint; use crate::{Event, Shared, Target}; @@ -35,6 +36,9 @@ const MAX_ENVELOPE_LEN: usize = 65_536; // Waiting out the whole timeout would hold a connection that the authority has already finished with. // When changing `close`, keep reading until the authority's side ends. const CLOSE_TIMEOUT: Duration = Duration::from_secs(5); +/// The largest clock offset, in seconds, that a `clock_skew` refusal makes the agent sign a retry with, so that a +/// signed opening is never dated more than a day from the agent's own clock. +const MAX_CLOCK_SKEW_OFFSET_SECS: u64 = 86_400; pub(crate) async fn run( shared: Arc, @@ -173,11 +177,11 @@ fn push_event(message: ServerMessage) -> Option { match message.payload? { Payload::RenewRequested(request) => Some(Event::RenewRequested { id: message.id, - reason: request.reason, + reason: loggable(&request.reason), }), Payload::Reconnect(request) => Some(Event::Reconnect { id: message.id, - reason: request.reason, + reason: loggable(&request.reason), }), Payload::ConfigUpdate(update) => Some(Event::ConfigUpdate { id: message.id, @@ -265,12 +269,13 @@ fn hello(target: &Target, challenge_id: &str, challenge: &[u8], nonce: &str) -> } /// Opens and authenticates a stream, and retries once at the authority's time when it refuses the opening with -/// `clock_skew` ([CONTRACT.agent-identity.md §6 "RFC 9421 profile"][contract]). +/// `clock_skew` and that time is within [`MAX_CLOCK_SKEW_OFFSET_SECS`] of the local time ([CONTRACT.agent-identity.md +/// §6 "RFC 9421 profile"][contract]). /// /// [contract]: ../../../docs/agent-identity/CONTRACT.agent-identity.md#6-rfc-9421-profile async fn authenticated_stream(shared: &Shared, target: &Target) -> Result { match authenticated_stream_at(shared, target, 0).await { - Err(closure) => match clock_offset(&closure) { + Err(closure) => match clock_offset(&closure, OffsetDateTime::now_utc()) { Some(offset) => authenticated_stream_at(shared, target, offset).await, None => Err(closure), }, @@ -290,13 +295,20 @@ async fn authenticated_stream_at(shared: &Shared, target: &Target, clock_offset: .unwrap_or_else(|_| Err(Closure::outage("no Welcome within the opening timeout"))) } -/// Returns `server_time − local_time` in seconds for a `clock_skew` refusal with a `server-time` trailer. -fn clock_offset(closure: &Closure) -> Option { - if closure.error_code.as_deref() != Some("clock_skew") { +/// Returns `server_time − local_time` in seconds for a refusal with status `UNAUTHENTICATED`, `error-code: clock_skew` +/// and a `server-time` trailer ([CONTRACT.agent-identity.md §7.4 "Termination"][contract]), unless its magnitude +/// exceeds [`MAX_CLOCK_SKEW_OFFSET_SECS`]. +/// +/// [contract]: ../../../docs/agent-identity/CONTRACT.agent-identity.md#74-termination +fn clock_offset(closure: &Closure, local_time: OffsetDateTime) -> Option { + if !(closure.grpc_code == Some(Code::Unauthenticated) + && closure.error_code.as_ref().map(ErrorCode::as_str) == Some("clock_skew")) + { return None; } let server_time = OffsetDateTime::parse(closure.server_time.as_deref()?, &Rfc3339).ok()?; - Some(server_time.unix_timestamp() - OffsetDateTime::now_utc().unix_timestamp()) + Some(server_time.unix_timestamp() - local_time.unix_timestamp()) + .filter(|offset| offset.unsigned_abs() <= MAX_CLOCK_SKEW_OFFSET_SECS) } /// Sends the signed opening request, with its `created` time offset by `clock_offset` seconds, and waits for the @@ -359,3 +371,76 @@ pub(crate) fn channel_proof_input(challenge: &[u8], connect_nonce: &str) -> Vec< input.extend_from_slice(connect_nonce.as_bytes()); input } + +#[cfg(test)] +mod tests { + use agent_channel_proto::{Reconnect, RenewRequested}; + use rstest::rstest; + + use super::*; + use crate::closure::TransientKind; + + /// A transient refusal with `error-code: clock_skew`, as the authority sends it at a skewed opening. + fn clock_skew_refusal(grpc_code: Code, server_time: &str) -> Closure { + Closure { + class: ClosureClass::Transient(TransientKind::Outage), + grpc_code: Some(grpc_code), + error_code: Some(ErrorCode::Code("clock_skew".to_owned())), + server_time: Some(server_time.to_owned()), + reason: "clock skew".to_owned(), + } + } + + fn local_time() -> OffsetDateTime { + OffsetDateTime::parse("2026-10-21T00:00:00Z", &Rfc3339).expect("RFC 3339 time") + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §signatures + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §channel-termination + // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Request authentication + #[rstest] + #[case::one_day_ahead("2026-10-22T00:00:00Z", 86_400)] + #[case::one_day_behind("2026-10-20T00:00:00Z", -86_400)] + fn a_clock_skew_refusal_within_a_day_gives_its_offset(#[case] server_time: &str, #[case] offset: i64) { + let closure = clock_skew_refusal(Code::Unauthenticated, server_time); + assert_eq!(clock_offset(&closure, local_time()), Some(offset)); + } + + // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Request authentication + #[rstest] + #[case::past_one_day_ahead("2026-10-22T00:00:01Z")] + #[case::past_one_day_behind("2026-10-19T23:59:59Z")] + #[case::years_ahead("9999-12-31T23:59:59Z")] + fn a_clock_skew_refusal_past_a_day_gives_no_offset(#[case] server_time: &str) { + let closure = clock_skew_refusal(Code::Unauthenticated, server_time); + assert_eq!(clock_offset(&closure, local_time()), None); + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §channel-termination + #[rstest] + #[case::permission_denied(Code::PermissionDenied)] + #[case::ok(Code::Ok)] + #[case::invalid_argument(Code::InvalidArgument)] + #[case::unavailable(Code::Unavailable)] + fn a_clock_skew_code_with_another_status_gives_no_offset(#[case] grpc_code: Code) { + let closure = clock_skew_refusal(grpc_code, "2026-10-21T02:00:00Z"); + assert_eq!(clock_offset(&closure, local_time()), None); + } + + // oracle: intent crates/agent-channel-client/INTENT.md §Connection + #[test] + fn escapes_the_reason_of_a_push() { + let push = |payload| ServerMessage { + id: "d6a1ed1a-6c5b-4f87-9d2a-5d1d3c1f8e10".to_owned(), + correlation_id: None, + payload: Some(payload), + }; + let reason = "admin\nINFO forged".to_owned(); + let logged = r"admin\nINFO forged".to_owned(); + + let renew = push_event(push(Payload::RenewRequested(RenewRequested { reason: reason.clone() }))); + assert!(matches!(renew, Some(Event::RenewRequested { reason, .. }) if reason == logged)); + let reconnect = push_event(push(Payload::Reconnect(Reconnect { reason }))); + assert!(matches!(reconnect, Some(Event::Reconnect { reason, .. }) if reason == logged)); + } +} diff --git a/crates/agent-channel-client/src/tests/mod.rs b/crates/agent-channel-client/src/tests/mod.rs index b777f6b07..e7ba84472 100644 --- a/crates/agent-channel-client/src/tests/mod.rs +++ b/crates/agent-channel-client/src/tests/mod.rs @@ -562,7 +562,7 @@ async fn reports_a_refused_opening_with_its_status_trailers_and_class( let (closure, _) = next_closure(&mut channel).await; assert_eq!(closure.class, class); assert_eq!(closure.grpc_code, Some(code)); - assert_eq!(closure.error_code.as_deref(), Some(error_code)); + assert_eq!(closure.error_code.as_ref().map(ErrorCode::as_str), Some(error_code)); assert_eq!(closure.server_time.as_deref(), Some(SERVER_TIME)); Ok(()) } @@ -596,7 +596,7 @@ async fn reports_the_closure_of_an_authenticated_stream( let (closure, _) = next_closure(&mut channel).await; assert_eq!(closure.class, class); assert_eq!(closure.grpc_code, Some(code)); - assert_eq!(closure.error_code.as_deref(), error_code); + assert_eq!(closure.error_code.as_ref().map(ErrorCode::as_str), error_code); Ok(()) } @@ -947,7 +947,7 @@ async fn a_second_clock_skew_refusal_is_transient() -> anyhow::Result<()> { let (closure, _) = next_closure(&mut channel).await; assert_eq!(closure.class, ClosureClass::Transient(TransientKind::Outage)); - assert_eq!(closure.error_code.as_deref(), Some("clock_skew")); + assert_eq!(closure.error_code.as_ref().map(ErrorCode::as_str), Some("clock_skew")); Ok(()) } @@ -1545,7 +1545,7 @@ async fn drops_an_acknowledgement_of_a_closed_stream() -> anyhow::Result<()> { fn assert_ok_closure_with_trailers(closure: &Closure) { assert_eq!(closure.class, ClosureClass::Transient(TransientKind::Outage)); assert_eq!(closure.grpc_code, Some(Code::Ok)); - assert_eq!(closure.error_code.as_deref(), Some("future_code")); + assert_eq!(closure.error_code.as_ref().map(ErrorCode::as_str), Some("future_code")); assert_eq!(closure.server_time.as_deref(), Some(SERVER_TIME)); } diff --git a/crates/agent-identity-conformance/src/agent.rs b/crates/agent-identity-conformance/src/agent.rs index 7f0593347..17e3302fc 100644 --- a/crates/agent-identity-conformance/src/agent.rs +++ b/crates/agent-identity-conformance/src/agent.rs @@ -1051,7 +1051,12 @@ impl AgentCase { fn write_pending(&mut self, token: &str) -> anyhow::Result<()> { self.add_token(token); let path = self.pending_path(); - std::fs::create_dir_all(path.parent().context("pending parent")?)?; + let parent = path.parent().context("pending parent")?; + // Some cases write a pending file before the agent's first start, and the agent refuses identity directories owned by another account. + #[cfg(windows)] + crate::windows::create_protected_dirs(parent)?; + #[cfg(not(windows))] + std::fs::create_dir_all(parent)?; write_pending_file(&path, &serde_json::to_vec(&json!({ "version": 1, "token": token }))?)?; #[cfg(windows)] crate::windows::pending_acl_is_protected(&path)?; @@ -2888,8 +2893,15 @@ pub(crate) async fn a_renew_transient_failures_retry_without_state_change(ctx: C // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §errors // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §agent-renewal // oracle: intent crates/agent-identity/INTENT.md §Stored identities - with_agent(ctx, None, true, async move |case| { + // oracle: intent devolutions-agent/src/identity/INTENT.md §Configuration + with_agent(ctx, None, false, async move |case| { let target = case.ctx.target.clone(); + // The three renewals follow one another in one run, faster than the agent's wait between renewals of one identity (`crates/agent-identity/INTENT.md`, "Schedules"), so the case removes that wait. + let config_path = case.path().join("agent.json"); + let mut config: Value = serde_json::from_slice(&std::fs::read(&config_path)?)?; + config["__debug__"]["identity"]["min_renewal_interval_secs"] = json!(0); + std::fs::write(&config_path, serde_json::to_vec_pretty(&config)?)?; + case.start().await?; let (id, initial) = case.enroll_new(&target, 1).await?; let authority = field(&initial, "authority_id")?.to_owned(); case.until_identity(&target, &id, "working channel for renewal faults", |record| { @@ -3416,15 +3428,7 @@ pub(crate) async fn a_cli_identity_enroll_writes_pending_file(ctx: Context) -> a with_agent(ctx, None, false, async move |case| { let token = new_token(case, 1).await?; case.add_token(&token.text); - let output = Command::new(&case.ctx.agent_bin) - .args(["identity", "enroll", &token.text]) - .env("DAGENT_CONFIG_PATH", case.path()) - .kill_on_drop(true) - .output() - .await - .context("run identity enroll CLI")?; - std::fs::write(case.path().join("cli-stdout.log"), &output.stdout)?; - std::fs::write(case.path().join("cli-stderr.log"), &output.stderr)?; + let output = enroll_cli_from_stdin(case, format!("{}\n", token.text).as_bytes()).await?; ensure!(output.status.success(), "identity enroll CLI rejected a valid token"); ensure!( case.pending_path().exists(), @@ -3504,25 +3508,9 @@ pub(crate) async fn a_cli_identity_enroll_writes_pending_file(ctx: Context) -> a ), ]; for (reason, candidate) in invalid { - let result = tokio::time::timeout( - Duration::from_secs(10), - Command::new(&case.ctx.agent_bin) - .args(["identity", "enroll", &candidate]) - .env("DAGENT_CONFIG_PATH", case.path()) - .kill_on_drop(true) - .output(), - ) - .await - .with_context(|| format!("identity enroll CLI hung for {reason}"))??; - use std::io::Write as _; - std::fs::OpenOptions::new() - .append(true) - .open(case.path().join("cli-stdout.log"))? - .write_all(&result.stdout)?; - std::fs::OpenOptions::new() - .append(true) - .open(case.path().join("cli-stderr.log"))? - .write_all(&result.stderr)?; + let result = enroll_cli_from_stdin(case, format!("{candidate}\n").as_bytes()) + .await + .with_context(|| format!("identity enroll CLI for {reason}"))?; ensure!(!result.status.success(), "identity enroll CLI accepted {reason}"); ensure!( !case.pending_path_for(&candidate).exists() && !case.in_progress_path_for(&candidate).exists(), @@ -3535,16 +3523,30 @@ pub(crate) async fn a_cli_identity_enroll_writes_pending_file(ctx: Context) -> a } async fn enroll_cli_from_stdin(case: &AgentCase, input: &[u8]) -> anyhow::Result { - let output = tokio::time::timeout(Duration::from_secs(10), async { - let mut child = Command::new(&case.ctx.agent_bin) - .args(["identity", "enroll", "-"]) - .env("DAGENT_CONFIG_PATH", case.path()) + enroll_cli(&case.ctx.agent_bin, case.path(), &["-"], input, Duration::from_secs(10)).await +} + +/// Runs `identity enroll` followed by `args` against the case configuration in `case_dir`, writing `input` to its standard input. +/// +/// Appends the command's output to the case's CLI logs, which the token audit then reads. +async fn enroll_cli( + agent_bin: &Path, + case_dir: &Path, + args: &[&str], + input: &[u8], + timeout: Duration, +) -> anyhow::Result { + let output = tokio::time::timeout(timeout, async { + let mut child = Command::new(agent_bin) + .args(["identity", "enroll"]) + .args(args) + .env("DAGENT_CONFIG_PATH", case_dir) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .kill_on_drop(true) .spawn() - .context("start identity enroll CLI with stdin")?; + .context("start identity enroll CLI")?; let mut stdin = child.stdin.take().context("identity enroll CLI has no stdin")?; if let Err(error) = stdin.write_all(input).await { ensure!( @@ -3556,24 +3558,24 @@ async fn enroll_cli_from_stdin(case: &AgentCase, input: &[u8]) -> anyhow::Result child.wait_with_output().await.context("wait for identity enroll CLI") }) .await - .context("identity enroll CLI with stdin timed out")??; + .context("identity enroll CLI timed out")??; use std::io::Write as _; for (name, contents) in [("cli-stdout.log", &output.stdout), ("cli-stderr.log", &output.stderr)] { std::fs::OpenOptions::new() .create(true) .append(true) - .open(case.path().join(name))? + .open(case_dir.join(name))? .write_all(contents)?; } Ok(output) } -fn assert_no_pending_files(case: &AgentCase) -> anyhow::Result<()> { +fn assert_no_pending_files(case: &AgentCase, input: &str) -> anyhow::Result<()> { let dir = case.path().join("identity").join("pending"); if dir.exists() { ensure!( std::fs::read_dir(dir)?.next().is_none(), - "identity enroll CLI created pending or in-progress state for invalid stdin" + "identity enroll CLI created pending or in-progress state for {input}" ); } Ok(()) @@ -3621,13 +3623,61 @@ pub(crate) async fn a_cli_identity_enroll_stdin(ctx: Context) -> anyhow::Result< ] { let output = enroll_cli_from_stdin(case, &input).await?; ensure!(!output.status.success(), "identity enroll CLI accepted {name} on stdin"); - assert_no_pending_files(case)?; + assert_no_pending_files(case, &format!("{name} on stdin"))?; } Ok(()) }) .await } +pub(crate) async fn a_cli_identity_enroll_token_argument_refused(ctx: Context) -> anyhow::Result<()> { + // oracle: intent devolutions-agent/src/identity/INTENT.md §CLI + // oracle: intent crates/agent-identity/INTENT.md §Pending-enrollment files + with_agent(ctx, None, false, async move |case| { + let token = new_token(case, 1).await?; + case.add_token(&token.text); + let line = format!("{}\n", token.text); + // Each run differs from the accepting control below only in its arguments, standard input carrying the same token. + for (name, args) in [ + ("the token as the argument", &[token.text.as_str()][..]), + ("the token after -", &["-", token.text.as_str()][..]), + ] { + let output = enroll_cli( + &case.ctx.agent_bin, + case.path(), + args, + line.as_bytes(), + Duration::from_secs(10), + ) + .await + .with_context(|| format!("identity enroll CLI with {name}"))?; + ensure!(!output.status.success(), "identity enroll CLI accepted {name}"); + assert_no_pending_files(case, name)?; + for (stream, contents) in [("stdout", &output.stdout), ("stderr", &output.stderr)] { + assert_no_token_fragments( + contents, + &token.text, + Path::new(&format!("identity enroll CLI {stream} for {name}")), + )?; + } + } + // The refusals above are for the arguments alone: the same token on standard input is accepted. + let output = enroll_cli_from_stdin(case, line.as_bytes()).await?; + ensure!( + output.status.success(), + "identity enroll CLI rejected the token on stdin after refusing it as an argument" + ); + let pending = case.pending_path(); + ensure!( + pending.exists(), + "identity enroll CLI did not write the pending file for the token on stdin" + ); + std::fs::remove_file(pending)?; + Ok(()) + }) + .await +} + pub(crate) async fn a_renewal_happy_path(ctx: Context) -> anyhow::Result<()> { // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §agent-renewal // oracle: intent crates/agent-identity/INTENT.md §Schedules diff --git a/crates/agent-identity-conformance/src/main.rs b/crates/agent-identity-conformance/src/main.rs index 350bc2243..c8b34d0a6 100644 --- a/crates/agent-identity-conformance/src/main.rs +++ b/crates/agent-identity-conformance/src/main.rs @@ -963,6 +963,7 @@ const TESTS: &[Test] = &[ a!(a_rejected_identity_replaced).channel(), a!(a_cli_identity_enroll_writes_pending_file).cli_only(), a!(a_cli_identity_enroll_stdin).cli_only(), + a!(a_cli_identity_enroll_token_argument_refused).cli_only(), a!(a_renewal_happy_path).channel().need(Need::LongLeaves), a!(a_renewal_lost_response_retried, mock_only).channel(), a!(a_renewal_lost_confirm_response_retried, mock_only).channel(), diff --git a/crates/agent-identity-conformance/src/system_fixture.rs b/crates/agent-identity-conformance/src/system_fixture.rs index a8ac01d8c..a56031e4f 100644 --- a/crates/agent-identity-conformance/src/system_fixture.rs +++ b/crates/agent-identity-conformance/src/system_fixture.rs @@ -1,4 +1,3 @@ -use std::io::Write as _; use std::path::{Path, PathBuf}; use std::process::{Output, Stdio}; use std::time::{Duration, Instant}; @@ -7,7 +6,7 @@ use anyhow::{Context as _, ensure}; use tokio::io::{AsyncReadExt as _, AsyncWriteExt as _}; use tokio::net::TcpListener; -use super::{AgentCase, POLL, WAIT, new_token, reset_mocks}; +use super::{AgentCase, POLL, WAIT, enroll_cli, new_token, reset_mocks}; use crate::Context; use crate::client::{Target, Token}; use crate::dvls::IdentityQuery; @@ -346,24 +345,17 @@ try {{\r\n\ async fn submit_token(&self, token: &str) -> anyhow::Result<()> { let started = Instant::now(); + let line = format!("{token}\n"); loop { - let output = tokio::time::timeout( + let output = enroll_cli( + &self.request.agent_bin, + self.case.path(), + &["-"], + line.as_bytes(), Duration::from_secs(5), - tokio::process::Command::new(&self.request.agent_bin) - .args(["identity", "enroll", token]) - .env("DAGENT_CONFIG_PATH", self.case.path()) - .kill_on_drop(true) - .output(), ) .await - .context("identity enroll CLI timed out")? .context("run identity enroll CLI as administrator")?; - for (name, contents) in [("cli-stdout.log", &output.stdout), ("cli-stderr.log", &output.stderr)] { - std::fs::OpenOptions::new() - .append(true) - .open(self.case.path().join(name))? - .write_all(contents)?; - } if output.status.success() { return Ok(()); } diff --git a/crates/agent-identity-conformance/src/windows.rs b/crates/agent-identity-conformance/src/windows.rs index 04b56f4b0..a1a7e7529 100644 --- a/crates/agent-identity-conformance/src/windows.rs +++ b/crates/agent-identity-conformance/src/windows.rs @@ -26,7 +26,9 @@ use windows::Win32::Security::{ DACL_SECURITY_INFORMATION, GetSecurityDescriptorControl, OBJECT_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, SE_DACL_PROTECTED, SECURITY_ATTRIBUTES, }; -use windows::Win32::Storage::FileSystem::{CREATE_ALWAYS, CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_MODE}; +use windows::Win32::Storage::FileSystem::{ + CREATE_ALWAYS, CreateDirectoryW, CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_MODE, +}; use windows::Win32::System::JobObjects::{ AssignProcessToJobObject, CreateJobObjectW, JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, JobObjectExtendedLimitInformation, SetInformationJobObject, @@ -225,12 +227,15 @@ pub(crate) fn write_pending(path: &Path, contents: &[u8]) -> anyhow::Result<()> write_protected_file(path, &encrypted) } -pub(crate) fn write_protected_file(path: &Path, contents: &[u8]) -> anyhow::Result<()> { +/// The security descriptor of the files and directories the suite protects: owned by the current user, with a protected DACL granting full control to SYSTEM and the current user only. +/// +/// `ace_flags` are the SDDL inheritance flags of every entry. +fn protected_descriptor(ace_flags: &str) -> anyhow::Result { let current_user = current_user_sid()?; let sddl = if current_user == "S-1-5-18" { - "O:SYD:P(A;;FA;;;SY)".to_owned() + format!("O:SYD:P(A;{ace_flags};FA;;;SY)") } else { - format!("O:{current_user}D:P(A;;FA;;;SY)(A;;FA;;;{current_user})") + format!("O:{current_user}D:P(A;{ace_flags};FA;;;SY)(A;{ace_flags};FA;;;{current_user})") }; let sddl = wide(std::ffi::OsStr::new(&sddl)); let mut raw = PSECURITY_DESCRIPTOR::default(); @@ -243,12 +248,54 @@ pub(crate) fn write_protected_file(path: &Path, contents: &[u8]) -> anyhow::Resu None, )? }; - let descriptor = SecurityDescriptor(raw); - let attributes = SECURITY_ATTRIBUTES { + Ok(SecurityDescriptor(raw)) +} + +fn security_attributes(descriptor: &SecurityDescriptor) -> anyhow::Result { + Ok(SECURITY_ATTRIBUTES { nLength: u32::try_from(size_of::())?, lpSecurityDescriptor: descriptor.0.0, bInheritHandle: BOOL(0), - }; + }) +} + +/// Creates each missing directory of `path`, owned by the current user, with the pending files' protected DACL. +/// +/// Under UAC elevation a new directory's default owner is Administrators, and the agent refuses an identity directory owned by another account. +/// Existing directories are left as they are, so a directory the agent provisioned keeps its owner and DACL. +/// Every entry is inherited by files and subdirectories, so a file created inside without its own descriptor gets no access beyond them. +pub(crate) fn create_protected_dirs(path: &Path) -> anyhow::Result<()> { + let missing = path + .ancestors() + .take_while(|dir| { + !dir.as_os_str().is_empty() + && std::fs::symlink_metadata(dir).is_err_and(|error| error.kind() == std::io::ErrorKind::NotFound) + }) + .collect::>(); + if missing.is_empty() { + return Ok(()); + } + let descriptor = protected_descriptor("OICI")?; + let attributes = security_attributes(&descriptor)?; + for dir in missing.into_iter().rev() { + let wide_dir = wide(dir.as_os_str()); + // SAFETY: Both the NUL-terminated path and security descriptor remain live until CreateDirectoryW returns. + let created = unsafe { CreateDirectoryW(PCWSTR::from_raw(wide_dir.as_ptr()), Some(&attributes)) }; + // UNTESTED: A directory created concurrently since the check above, such as by the agent provisioning at start-up, is left as it is. + // Changing its owner or DACL would alter a directory the agent owns, which it may then refuse or have to reset. + // When changing this, keep accepting an existing directory here without setting its security descriptor. + if let Err(error) = created + && !std::fs::metadata(dir).is_ok_and(|metadata| metadata.is_dir()) + { + return Err(anyhow::Error::from(error).context(format!("create directory {}", dir.display()))); + } + } + Ok(()) +} + +pub(crate) fn write_protected_file(path: &Path, contents: &[u8]) -> anyhow::Result<()> { + let descriptor = protected_descriptor("")?; + let attributes = security_attributes(&descriptor)?; let path = wide(path.as_os_str()); // SAFETY: Both the NUL-terminated path and security descriptor remain live until CreateFileW returns. let handle = unsafe { @@ -956,6 +1003,102 @@ mod tests { Ok(()) } + /// The SDDL of the parts of `path`'s security descriptor that `info` selects. + fn path_sddl(path: &Path, info: OBJECT_SECURITY_INFORMATION) -> anyhow::Result { + let path = wide(path.as_os_str()); + let mut raw = PSECURITY_DESCRIPTOR::default(); + // SAFETY: path is NUL-terminated and raw receives an allocated security descriptor. + let result = unsafe { + GetNamedSecurityInfoW( + PCWSTR::from_raw(path.as_ptr()), + SE_FILE_OBJECT, + info, + None, + None, + None, + None, + &mut raw, + ) + }; + ensure!(result.0 == 0, "could not read security descriptor ({})", result.0); + let descriptor = SecurityDescriptor(raw); + let mut sddl = PWSTR::null(); + // SAFETY: The descriptor is valid and the output string is owned until freed with LocalFree. + unsafe { + ConvertSecurityDescriptorToStringSecurityDescriptorW(descriptor.0, SDDL_REVISION_1, info, &mut sddl, None)? + }; + // SAFETY: The conversion returned a NUL-terminated string allocated for the caller. + let text = unsafe { sddl.to_string() }; + // SAFETY: LocalFree releases the SDDL string allocated by the conversion. + unsafe { LocalFree(Some(HLOCAL(sddl.0.cast()))) }; + text.map_err(Into::into) + } + + /// The DACL control flags and sorted ACEs of a DACL SDDL. + fn dacl_parts(dacl: &str) -> anyhow::Result<(String, Vec)> { + let (control, aces) = dacl + .strip_prefix("D:") + .and_then(|rest| rest.split_once('(')) + .with_context(|| format!("DACL has no entries: {dacl}"))?; + let mut aces = aces + .trim_end_matches(')') + .split(")(") + .map(ToOwned::to_owned) + .collect::>(); + aces.sort_unstable(); + Ok((control.to_owned(), aces)) + } + + /// The sorted ACEs that grant full control to SYSTEM and, unless it is SYSTEM, to `user`, with `flags`. + fn full_control_aces(flags: &str, user: &str) -> Vec { + let mut aces = vec![format!("A;{flags};FA;;;SY")]; + if user != "SY" { + aces.push(format!("A;{flags};FA;;;{user}")); + } + aces.sort_unstable(); + aces + } + + #[test] + fn protected_dirs_are_owned_by_the_current_user_and_leave_existing_dirs_unchanged() -> anyhow::Result<()> { + // oracle: intent crates/agent-identity/INTENT.md §Local state + // oracle: intent crates/agent-identity/INTENT.md §Pending-enrollment files + let scratch = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .join("target") + .join("agent-identity-windows-tests"); + std::fs::create_dir_all(&scratch)?; + let dir = tempfile::Builder::new().prefix("protected-dirs-").tempdir_in(scratch)?; + let existing = dir.path().join("identity"); + std::fs::create_dir(&existing)?; + let existing_before = path_sddl(&existing, OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION)?; + let pending = existing.join("pending").join("nested"); + + create_protected_dirs(&pending)?; + + ensure!( + path_sddl(&existing, OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION)? == existing_before, + "an existing directory's owner or DACL changed" + ); + let user = canonical_sddl_sid(¤t_user_sid()?)?; + for created in [existing.join("pending"), pending.clone()] { + assert_eq!(path_sddl(&created, OWNER_SECURITY_INFORMATION)?, format!("O:{user}")); + let (control, aces) = dacl_parts(&path_sddl(&created, DACL_SECURITY_INFORMATION)?)?; + ensure!( + control.contains('P'), + "created directory DACL is not protected: D:{control}" + ); + assert_eq!(aces, full_control_aces("OICI", &user)); + } + // A file created inside without its own descriptor inherits no access beyond SYSTEM and the current user. + let file = pending.join("inherited.txt"); + std::fs::write(&file, b"")?; + let (_, aces) = dacl_parts(&path_sddl(&file, DACL_SECURITY_INFORMATION)?)?; + assert_eq!(aces, full_control_aces("ID", &user)); + Ok(()) + } + #[test] fn pending_acl_rejects_extra_allow_ace() { // oracle: intent crates/agent-identity/INTENT.md §Pending-enrollment files diff --git a/crates/agent-identity/INTENT.md b/crates/agent-identity/INTENT.md index 653bdf6be..86b3b1535 100644 --- a/crates/agent-identity/INTENT.md +++ b/crates/agent-identity/INTENT.md @@ -26,6 +26,7 @@ - An identity renews `2/3 × lifetime` after its certificate's anchor, plus a jitter drawn uniformly from `[0, lifetime / 12]`, or as soon as the authority requests a renewal. The lifetime is `notAfter − notBefore`; the anchor is the earlier of `notBefore` and the moment the agent stored or loaded the certificate in its current run, so that an authority clock ahead of the agent's doesn't delay renewal. That moment is not persisted, so after a restart the anchor uses the load time; it still never passes `notBefore`, and the agent keeps no extra state for a clock skew that only matters until `notBefore` arrives. + A renewal starts no sooner than 10 minutes after the identity's previous renewal completed in the current run, whether the schedule, the authority or a `certificate_expired` refusal asks for it, so that an authority cannot keep the agent renewing in a tight loop, for example with a certificate whose lifetime is zero; `renewal_after_secs` is exempt, and a test-only `__debug__.identity` setting shortens the wait. - A transient failure is retried after `2^n` seconds for the `n`-th consecutive failure, multiplied by a jitter drawn uniformly from `[0.5, 1.5]`, waiting at least a `Retry-After` delay when the authority sends one, and never longer than 1 h. Retry state lives in memory, so a restart retries at once. - Check-in runs only while the identity has no authenticated stream: @@ -37,13 +38,15 @@ The location and format of the files below are stable, because the conformance tester stands in for the installer and observes the agent through them. `` means the agent's configured data directory. -- The agent provisions its identity directories and, at start-up, resets a wrong owner or wrong permissions on them, without changing any authority directory under them. +- The agent provisions its identity directories and, at start-up, resets wrong permissions on them, without changing any authority directory under them. + An identity directory owned by another account is never repaired, and stops all identity processing until it is fixed, because the account that created it can keep a handle that still changes it after a reset. - Before it uses stored identities, the agent checks that every `/identity/authorities//` directory is owned by its account and protected from other users. One that fails the check is never repaired, and stops all identity processing until it is fixed, because skipping it would let a local attacker hide or forge an identity, for example hide a rejected identity so that its token enrolls again. The running agent repeats the check before every use of its state and on each pending-directory poll, stops every identity and its streams while any directory fails it, and resumes without a restart once all pass. A directory passes only if it is owned by the agent's account and is, on Unix, closed to group and other, or on Windows, protected by exactly the DACL the agent provisions; a link, a reparse point, a non-directory and, on a case-insensitive file system, any case variant of its name are all checked. - An `identity.json` that fails the same check is treated like an untrusted authority directory: never repaired, never read as absent, and it stops all identity processing until it is fixed. -- The agent ignores and keeps a rejection marker or in-progress record that it doesn't own or that other users can access: trusting one would let a local attacker reject a token or supply its enrollment key, while an attacker able to write one could delete it anyway, so ignoring it opens nothing new. +- The agent ignores a rejection marker or in-progress record that it doesn't own or that other users can access, as if it were absent, and never deletes one as cleanup; its own write of that file replaces it. + Trusting one would let a local attacker reject a token or supply its enrollment key, while an attacker able to write one could delete it anyway, so ignoring it opens nothing new. - Every state change holds an exclusive lock on the identity directory: the file `/identity/.agent-identity.lock` on Linux and macOS, and on Windows an unshared handle on `/identity` itself; on Windows, a busy lock is retried for 30 s. - Size limits: a pending file holds at most 16 KiB of plaintext, an in-progress record 16 KiB, `identity.json` 2 MiB, and a rejection marker 1 KiB. diff --git a/crates/agent-identity/src/client.rs b/crates/agent-identity/src/client.rs index 1b7af5360..aac2a1c91 100644 --- a/crates/agent-identity/src/client.rs +++ b/crates/agent-identity/src/client.rs @@ -35,6 +35,10 @@ use crate::token::Token; const MAX_RESPONSE_BYTES: usize = 1024 * 1024; +/// The largest clock offset, in seconds, that a `clock_skew` answer makes the agent sign a retry with, so that a +/// signed request is never dated more than a day from the agent's own clock. +const MAX_CLOCK_SKEW_OFFSET_SECS: u64 = 86_400; + pub type ClientResult = Result; #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -372,11 +376,16 @@ impl Client { let first = self .send_signed(&url, tag, signer, body, clock_offset_secs.unwrap_or_default()) .await; - if let Err(ClientError::ClockSkew { server_time }) = first { - let offset = server_time.unix_timestamp() - OffsetDateTime::now_utc().unix_timestamp(); - self.send_signed(&url, tag, signer, body, offset).await - } else { - first + let retry_offset = match &first { + Err(ClientError::ClockSkew { server_time }) => { + Some(server_time.unix_timestamp() - OffsetDateTime::now_utc().unix_timestamp()) + .filter(|offset| offset.unsigned_abs() <= MAX_CLOCK_SKEW_OFFSET_SECS) + } + _ => None, + }; + match retry_offset { + Some(offset) => self.send_signed(&url, tag, signer, body, offset).await, + None => first, } } @@ -1219,6 +1228,50 @@ mod tests { Ok(()) } + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §signatures + // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Request authentication + #[rstest] + #[case::ahead_one_hour(3_600)] + #[case::behind_one_hour(-3_600)] + #[case::ahead_almost_a_day(86_400 - 60)] + #[case::behind_almost_a_day(-(86_400 - 60))] + #[tokio::test] + async fn a_clock_skew_within_a_day_is_retried_once_with_the_server_time_offset( + #[case] skew_secs: i64, + ) -> anyhow::Result<()> { + let authority = SkewedAuthority::start(skew_secs)?; + + let error = Request::CheckIn + .send_to(ProxyConfig::Off, &authority.base_url()) + .await?; + + assert!(matches!(error, Some(ClientError::ClockSkew { .. })), "{error:?}"); + let created = authority.created(); + assert_eq!(created.len(), 2, "{created:?}"); + // The retry is dated `server_time − local_time` from the local clock, give or take the seconds the test takes. + let expected = OffsetDateTime::now_utc().unix_timestamp() + skew_secs; + assert!((created[1] - expected).abs() <= 5, "{created:?}, expected {expected}"); + Ok(()) + } + + // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Request authentication + #[rstest] + #[case::ahead_a_day_and_a_minute(86_400 + 60)] + #[case::behind_a_day_and_a_minute(-(86_400 + 60))] + #[case::ahead_a_year(365 * 86_400)] + #[tokio::test] + async fn a_clock_skew_beyond_a_day_is_returned_without_a_retry(#[case] skew_secs: i64) -> anyhow::Result<()> { + let authority = SkewedAuthority::start(skew_secs)?; + + let error = Request::CheckIn + .send_to(ProxyConfig::Off, &authority.base_url()) + .await?; + + assert!(matches!(error, Some(ClientError::ClockSkew { .. })), "{error:?}"); + assert_eq!(authority.created().len(), 1); + Ok(()) + } + #[derive(Clone, Copy, Debug)] enum Request { Enroll, @@ -1233,19 +1286,26 @@ mod tests { /// Sends this request to `authority` with `client`, and returns its error. async fn send_with(self, client: Client, authority: &CountingListener) -> anyhow::Result> { + self.send_with_to(client, &authority.base_url()).await + } + + /// Sends this request to `base_url` through `proxy`, and returns its error. + async fn send_to(self, proxy: ProxyConfig, base_url: &str) -> anyhow::Result> { + self.send_with_to(Client::new(proxy, None)?, base_url).await + } + + /// Sends this request to `base_url` with `client`, and returns its error. + async fn send_with_to(self, client: Client, base_url: &str) -> anyhow::Result> { let key = TestKey(SigningKey::from_slice(&[7u8; 32])?); let error = match self { Self::Enroll => { - let bag = URL_SAFE_NO_PAD.encode(format!(r#"{{"u":"{}"}}"#, authority.base_url())); + let bag = URL_SAFE_NO_PAD.encode(format!(r#"{{"u":"{base_url}"}}"#)); let token = Token::parse(&format!("dvaet1.{bag}.{}", "A".repeat(43)))?; client.enroll(&token, "csr", &Metadata::new(), &key).await.err() } Self::CheckIn => { let signer = RequestSigner::new(Arc::new(key), b"leaf certificate"); - client - .check_in(&authority.base_url(), &signer, &Metadata::new(), None) - .await - .err() + client.check_in(base_url, &signer, &Metadata::new(), None).await.err() } }; Ok(error) @@ -1299,6 +1359,95 @@ mod tests { } } + /// A loopback authority that answers every request with `clock_skew`, giving a `server_time` `skew_secs` seconds + /// from the local clock, and records the `created` signature parameter of each request. + /// + /// It speaks plain HTTP, which the client sends to an `http` base URL, because the dates of a signed request don't + /// depend on TLS. + struct SkewedAuthority { + port: u16, + created: Arc>>, + } + + impl SkewedAuthority { + fn start(skew_secs: i64) -> anyhow::Result { + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + let port = listener.local_addr()?.port(); + let created = Arc::new(Mutex::new(Vec::new())); + let recorded = Arc::clone(&created); + std::thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { + return; + }; + let Some(head) = read_request_head(&mut stream) else { + continue; + }; + if let Some(value) = signature_created(&head) { + recorded.lock().expect("created list not poisoned").push(value); + } + let server_time = + OffsetDateTime::from_unix_timestamp(OffsetDateTime::now_utc().unix_timestamp() + skew_secs) + .expect("valid server time") + .format(&Rfc3339) + .expect("formattable server time"); + let body = format!(r#"{{"error":"clock_skew","message":"skewed","server_time":"{server_time}"}}"#); + let response = format!( + "HTTP/1.1 401 Unauthorized\r\ncontent-type: application/json\r\ncontent-length: {}\r\nconnection: close\r\n\r\n{body}", + body.len() + ); + let _ = std::io::Write::write_all(&mut stream, response.as_bytes()); + } + }); + Ok(Self { port, created }) + } + + fn base_url(&self) -> String { + format!("http://127.0.0.1:{}", self.port) + } + + fn created(&self) -> Vec { + self.created.lock().expect("created list not poisoned").clone() + } + } + + /// Reads a request's head and consumes its body, so that closing the connection doesn't reset it before the client + /// reads the response. + fn read_request_head(stream: &mut std::net::TcpStream) -> Option { + let mut received = Vec::new(); + let mut chunk = [0u8; 4096]; + let head_end = loop { + if let Some(position) = received.windows(4).position(|window| window == b"\r\n\r\n") { + break position + 4; + } + let read = std::io::Read::read(stream, &mut chunk).ok().filter(|read| *read != 0)?; + received.extend_from_slice(&chunk[..read]); + }; + let head = String::from_utf8(received[..head_end].to_vec()).ok()?; + let content_length = head + .lines() + .filter_map(|line| line.split_once(':')) + .find(|(name, _)| name.eq_ignore_ascii_case("content-length")) + .and_then(|(_, value)| value.trim().parse::().ok()) + .unwrap_or(0); + while received.len() < head_end + content_length { + let read = std::io::Read::read(stream, &mut chunk).ok().filter(|read| *read != 0)?; + received.extend_from_slice(&chunk[..read]); + } + Some(head) + } + + /// Returns the `created` parameter of a request head's `signature-input` field. + fn signature_created(head: &str) -> Option { + let (_, value) = head + .lines() + .filter_map(|line| line.split_once(':')) + .find(|(name, _)| name.eq_ignore_ascii_case("signature-input"))?; + let (_, after) = value.split_once(";created=")?; + let digits: String = after.chars().take_while(char::is_ascii_digit).collect(); + digits.parse().ok() + } + fn manual_proxy(proxy: &str, exclude: &[&str]) -> anyhow::Result { Ok(ProxyConfig::Manual(ManualProxyConfig { all: Some(Url::parse(proxy)?), diff --git a/crates/agent-identity/src/private_file.rs b/crates/agent-identity/src/private_file.rs index eb4ecdf1f..2f1b97272 100644 --- a/crates/agent-identity/src/private_file.rs +++ b/crates/agent-identity/src/private_file.rs @@ -56,6 +56,35 @@ pub(crate) fn is_untrusted(error: &anyhow::Error) -> bool { error.is::() } +/// An identity directory, `/identity` or its `authorities`, `keys`, `rejected` or `pending`, whose owner the +/// agent does not accept, such as another account, which provisioning never repairs. +/// +/// [`crates/agent-identity/INTENT.md`] ("Local state"): such a directory stops all identity processing until it is +/// fixed, because the account that created it can keep a handle that still changes it after a reset. +/// +/// [`crates/agent-identity/INTENT.md`]: ../INTENT.md +#[derive(Debug)] +pub struct ForeignIdentityDirectory { + pub path: Utf8PathBuf, + source: private_fs::Error, +} + +impl fmt::Display for ForeignIdentityDirectory { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + formatter, + "identity directory {} has an owner the agent does not accept; the agent never repairs it", + self.path + ) + } +} + +impl Error for ForeignIdentityDirectory { + fn source(&self) -> Option<&(dyn Error + 'static)> { + Some(&self.source) + } +} + fn untrusted() -> anyhow::Error { warn!("Ignore untrusted identity state file"); anyhow::Error::new(UntrustedStateFile) @@ -193,8 +222,8 @@ fn open_parent(path: &Utf8Path) -> anyhow::Result<(Dir, &str)> { /// that no directory created on the way can be replaced or redirected by another account. /// The installer and the configuration create `` in production, so this applies only where they didn't. /// -/// [`crates/agent-identity/INTENT.md`] ("Local state"): an identity directory with a wrong owner or wrong permissions is -/// reset, without changing anything below it. +/// [`crates/agent-identity/INTENT.md`] ("Local state"): an identity directory with wrong permissions is reset, without +/// changing anything below it, and one owned by another account is refused unchanged. /// /// [`crates/agent-identity/INTENT.md`]: ../INTENT.md pub(crate) fn provision_directories(data_dir: &Utf8Path, grant_current_user: bool) -> anyhow::Result { @@ -219,9 +248,19 @@ pub(crate) fn provision_directories(data_dir: &Utf8Path, grant_current_user: boo fn provision(parent: &Dir, name: &str, rule: &Rule) -> anyhow::Result { // A repair changes nothing on a directory that already follows its rule, and also covers one whose access // denies the agent or whose mode is narrower than the rule's. + // The owner is checked through the handle that the repair then writes through, so that the directory refused or + // repaired is the one checked. parent - .create_dir(name, rule, Existing::Repair) - .context("provision identity directory") + .create_dir(name, rule, Existing::RepairAccess) + .map_err(|error| match error.kind() { + private_fs::ErrorKind::Refused(private_fs::Refusal::Owner) => { + anyhow::Error::new(ForeignIdentityDirectory { + path: error.path().to_owned(), + source: error, + }) + } + _ => anyhow::Error::new(error).context("provision identity directory"), + }) } } @@ -764,7 +803,7 @@ mod tests { mod unix_tests { use std::fs; use std::io::Write as _; - use std::os::unix::fs::PermissionsExt as _; + use std::os::unix::fs::{MetadataExt as _, PermissionsExt as _}; use super::*; @@ -838,6 +877,46 @@ mod unix_tests { Ok(()) } + // oracle: intent crates/agent-identity/INTENT.md §Local state + #[test] + fn provisioning_resets_a_child_directory_open_to_group_and_other() -> anyhow::Result<()> { + let directory = TestDir::provisioned()?; + let authorities = directory.0.join("identity").join("authorities"); + fs::set_permissions(&authorities, fs::Permissions::from_mode(0o755))?; + + prepare_directories(&directory.0, true)?; + + assert_eq!(fs::metadata(&authorities)?.permissions().mode() & 0o777, 0o700); + Ok(()) + } + + // oracle: intent crates/agent-identity/INTENT.md §Local state + #[rstest::rstest] + #[case::identity("identity")] + #[case::authorities("identity/authorities")] + fn provisioning_refuses_an_identity_directory_owned_by_another_account(#[case] name: &str) -> anyhow::Result<()> { + let directory = TestDir::provisioned()?; + let path = directory.0.join(name); + // Wrong access as well, so that a repair would show in the mode even if it kept the owner. + fs::set_permissions(&path, fs::Permissions::from_mode(0o755))?; + if !give_to_another_account(&path)? { + return Ok(()); + } + let ownership = |path: &Utf8Path| { + fs::symlink_metadata(path).map(|metadata| (metadata.uid(), metadata.gid(), metadata.mode())) + }; + let before = ownership(&path)?; + + let error = prepare_directories(&directory.0, true).expect_err("provisioning accepted a foreign directory"); + + let foreign = error + .downcast_ref::() + .with_context(|| format!("the failure is not a foreign identity directory: {error:#}"))?; + assert_eq!(foreign.path, path); + assert_eq!(ownership(&path)?, before); + Ok(()) + } + // oracle: intent crates/agent-identity/INTENT.md §Pending-enrollment files #[test] fn a_pending_file_owned_by_another_account_is_ignored() -> anyhow::Result<()> { @@ -1081,6 +1160,44 @@ mod windows_tests { Ok(()) } + // oracle: intent crates/agent-identity/INTENT.md §Local state + #[rstest::rstest] + #[case::identity("identity")] + #[case::authorities("identity/authorities")] + fn provisioning_refuses_an_identity_directory_owned_by_another_account(#[case] name: &str) -> anyhow::Result<()> { + let directory = TestDir::provisioned()?; + let path = directory.0.join(name); + // Wrong access as well, so that a repair would show in the DACL even if it kept the owner. + let users = Sid::from_well_known(WinBuiltinUsersSid, None)?; + acl::set_dacl( + &path, + &[ + full(&acl::system_sid()?, INHERITED), + full(&acl::user_sid()?, INHERITED), + full(&users, INHERITED), + ], + )?; + let administrators = Sid::from_well_known(WinBuiltinAdministratorsSid, None)?; + if acl::set_owner(&path, &administrators).is_err() { + writeln!( + io::stderr(), + "SKIP: provisioning_refuses_an_identity_directory_owned_by_another_account needs an elevated token to \ + give a directory another owner" + )?; + return Ok(()); + } + let before = acl::test_security(&path)?; + + let error = prepare_directories(&directory.0, true).expect_err("provisioning accepted a foreign directory"); + + let foreign = error + .downcast_ref::() + .with_context(|| format!("the failure is not a foreign identity directory: {error:#}"))?; + assert_eq!(foreign.path, path); + assert_eq!(acl::test_security(&path)?, before); + Ok(()) + } + // oracle: intent crates/agent-identity/INTENT.md §Local state #[test] fn repairs_a_directory_with_an_extra_allow_principal() -> anyhow::Result<()> { @@ -1089,6 +1206,9 @@ mod windows_tests { fs::create_dir(&identity)?; let user = acl::user_sid()?; let system = acl::system_sid()?; + // An elevated token makes BUILTIN\Administrators the owner of what it creates, so the agent's ownership is set + // explicitly on every directory the test creates. + acl::set_owner(&identity, &user)?; let users = Sid::from_well_known(WinBuiltinUsersSid, None)?; acl::set_dacl( &identity, @@ -1100,21 +1220,21 @@ mod windows_tests { )?; // `crates/agent-identity/INTENT.md` ("Local state"): start-up resets the directory's permissions, so the // identities and keys stored beneath it must survive the repair. + let authorities = identity.join("authorities"); + let authority = authorities.join(Uuid::new_v4().to_string()); + let keys = identity.join("keys"); + for path in [&authorities, &authority, &keys] { + fs::create_dir(path)?; + acl::set_owner(path, &user)?; + } let state = [ + (authority.join("identity.json"), br#"{"version":1}"#.as_slice()), ( - identity - .join("authorities") - .join(Uuid::new_v4().to_string()) - .join("identity.json"), - br#"{"version":1}"#.as_slice(), - ), - ( - identity.join("keys").join("devolutions-agent-identity_test.p8"), + keys.join("devolutions-agent-identity_test.p8"), b"device key".as_slice(), ), ]; for (path, contents) in &state { - fs::create_dir_all(path.parent().context("test state file has no parent")?)?; fs::write(path, contents)?; } diff --git a/crates/agent-identity/src/schedule.rs b/crates/agent-identity/src/schedule.rs index 4cffa0f24..d8ddb1f86 100644 --- a/crates/agent-identity/src/schedule.rs +++ b/crates/agent-identity/src/schedule.rs @@ -56,6 +56,7 @@ pub fn check_in_interval(base: Duration, jitter: f64) -> Duration { mod tests { use std::time::UNIX_EPOCH; + use rstest::rstest; use serde_json::json; use super::*; @@ -179,4 +180,24 @@ mod tests { assert_eq!(check_in_interval(day, 0.5), day); assert_eq!(check_in_interval(day, 1.0), Duration::from_secs(95_040)); } + + /// `agent_channel_url` is read as text like a token's `u`, which holds only ASCII characters, and a URI holds no + /// white space or control character (RFC 3986 §2), which the `url` crate would remove or trim rather than refuse. + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §enroll + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §enrollment-token + // oracle: spec RFC 3986 §2 + #[rstest] + #[case::tab_in_host("https://example.test\t.evil.test/dvls")] + #[case::line_feed_in_host("https://example.test\n.evil.test/dvls")] + #[case::carriage_return_in_host("https://example.test\r.evil.test/dvls")] + #[case::space_in_path("https://example.test/dv ls")] + #[case::trailing_space("https://example.test/dvls ")] + #[case::nul_in_path("https://example.test/dv\u{0}ls")] + #[case::trailing_unit_separator("https://example.test/dvls\u{1f}")] + #[case::delete_in_path("https://example.test/dv\u{7f}ls")] + #[case::non_ascii_host("https://exämple.test/dvls")] + fn a_channel_url_with_white_space_a_control_or_a_non_ascii_character_is_malformed(#[case] url: &str) { + let config = json!({"version": 1, "revision": 1, "agent_channel_url": url}); + assert_eq!(channel_url(&config), ChannelUrl::Malformed); + } } diff --git a/crates/agent-identity/src/state.rs b/crates/agent-identity/src/state.rs index b98c832cd..3d90f10a0 100644 --- a/crates/agent-identity/src/state.rs +++ b/crates/agent-identity/src/state.rs @@ -10,7 +10,7 @@ use time::OffsetDateTime; use time::format_description::well_known::Rfc3339; use uuid::{Uuid, Variant}; -pub use crate::private_file::UntrustedStoredIdentity; +pub use crate::private_file::{ForeignIdentityDirectory, UntrustedStoredIdentity}; use crate::token::Token; use crate::{chain, pending, private_file, rejected, state_lock}; @@ -179,6 +179,21 @@ impl Store { private_file::check_stored_identities(&self.data_dir, self.acl_grant_current_user).map(drop) } + /// Runs the checks every state change runs before it writes, without taking the state lock: provisioning the + /// identity directories, which refuses one whose owner the agent does not accept with a + /// [`ForeignIdentityDirectory`], then [`Store::check_stored_identities`]. + /// + /// [`crates/agent-identity/INTENT.md`] ("Local state"): either refusal stops all identity processing until it is + /// fixed, so a running agent with no state change to make still notices one. + /// The lock is left out because there is nothing to write, and waiting on it would hold this check behind a state + /// change in progress or, on Windows, behind any outside handle on the identity directory. + /// + /// [`crates/agent-identity/INTENT.md`]: ../INTENT.md + pub fn check_state(&self) -> anyhow::Result<()> { + private_file::provision_directories(&self.data_dir, self.acl_grant_current_user)?; + self.check_stored_identities() + } + /// Takes the state lock once every stored identity passes [`Store::check_stored_identities`]. fn lock(&self) -> anyhow::Result { let lock = state_lock::acquire(&self.data_dir, self.acl_grant_current_user)?; diff --git a/crates/agent-identity/src/token.rs b/crates/agent-identity/src/token.rs index 8b4909a5d..9b0aa20e5 100644 --- a/crates/agent-identity/src/token.rs +++ b/crates/agent-identity/src/token.rs @@ -46,8 +46,7 @@ impl Token { .as_object() .and_then(|fields| fields.get("u")) .and_then(serde_json::Value::as_str) - // Certificates carry `u` as an `IA5String`, so it holds only ASCII characters. - .filter(|url| url.is_ascii() && is_base_url(url)) + .filter(|url| is_base_url(url)) .context("invalid enrollment authority URL")?; let base_url = base_url.trim_end_matches('/').to_owned(); Ok(Self { @@ -90,15 +89,17 @@ pub(crate) fn is_base_url(url: &str) -> bool { /// shared by a token's `u` ([CONTRACT.agent-identity.md §2 "Enrollment token"][token]) and `agent_channel_url` /// ([CONTRACT.agent-identity.md §5.2 "`POST enroll`"][enroll]). /// -/// The URL is read as text first: it begins with `https://`, compared as written, and contains no `@`, since any `@` counts -/// as a user name even with nothing before it. +/// The URL is read as text first: it holds only printable ASCII characters, begins with `https://`, compared as written, +/// and contains no `@`, since any `@` counts as a user name even with nothing before it. +/// Certificates carry the URL as an `IA5String`, so it holds only ASCII characters, and a URI never holds white space or +/// control characters, which the `url` crate removes or trims rather than refusing, joining the text around them. /// URL parsers disagree on forms such as `https://@host` or `https:host`, which the `url` crate accepts as `https` /// URLs without a user name, so the text decides before the parsed URL is checked. /// /// [token]: ../../../docs/agent-identity/CONTRACT.agent-identity.md#enrollment-token /// [enroll]: ../../../docs/agent-identity/CONTRACT.agent-identity.md#enroll pub(crate) fn parse_base_url(url: &str) -> Option { - if !url.starts_with("https://") || url.contains('@') { + if !url.bytes().all(|byte| byte.is_ascii_graphic()) || !url.starts_with("https://") || url.contains('@') { return None; } Url::parse(url).ok().filter(|url| { @@ -219,4 +220,25 @@ mod tests { // oracle: intent docs/agent-identity/INTENT.agent-identity.md §Security invariants assert!(!error.to_string().contains(&raw), "{error}"); } + + /// `u` is an absolute URL read as text, and a URI holds no white space or control character (RFC 3986 §2), which + /// the `url` crate would remove or trim rather than refuse. + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §enrollment-token + // oracle: spec RFC 3986 §2 + #[rstest] + #[case::tab_in_host("https://example.test\t.evil.test/dvls")] + #[case::line_feed_in_host("https://example.test\n.evil.test/dvls")] + #[case::carriage_return_in_host("https://example.test\r.evil.test/dvls")] + #[case::space_in_path("https://example.test/dv ls")] + #[case::trailing_space("https://example.test/dvls ")] + #[case::nul_in_path("https://example.test/dv\u{0}ls")] + #[case::trailing_unit_separator("https://example.test/dvls\u{1f}")] + #[case::delete_in_path("https://example.test/dv\u{7f}ls")] + fn rejects_a_base_url_with_white_space_or_a_control_character(#[case] u: &str) { + let raw = token_with_bag(&serde_json::json!({ "u": u }).to_string()); + assert!( + Token::parse(&raw).is_err(), + "a base URL with white space or a control character was accepted" + ); + } } diff --git a/crates/private-fs/src/lib.rs b/crates/private-fs/src/lib.rs index 9de5d742e..2a638c422 100644 --- a/crates/private-fs/src/lib.rs +++ b/crates/private-fs/src/lib.rs @@ -50,6 +50,8 @@ pub enum Existing { Check, /// The directory is brought into line with the rule. Repair, + /// The directory's access is brought into line with the rule, and an owner the rule doesn't allow is refused first. + RepairAccess, } /// Whether a rename may replace an existing entry. diff --git a/crates/private-fs/src/unix.rs b/crates/private-fs/src/unix.rs index 907be8a79..0001f17d1 100644 --- a/crates/private-fs/src/unix.rs +++ b/crates/private-fs/src/unix.rs @@ -129,14 +129,14 @@ pub(crate) fn create_dir(parent: &Dir, path: &Utf8Path, name: &str, rule: &Rule, let open = || open_at(parent, path, name, libc::O_RDONLY | libc::O_DIRECTORY, Kind::Directory); match open() { Err(error) if restore && is_permission_denied(&error) => { - restore_owner_access(parent, &c_name, rule, error)?; + restore_owner_access(parent, &c_name, rule, existing, error)?; open() } opened => opened, } }; // A directory that exists is checked or repaired, so only one that must be created can inherit the parent's ACL. - let (file, created) = match open(existing == Existing::Repair)? { + let (file, created) = match open(existing != Existing::Check)? { Some(file) => (file, false), None => { os::refuse_inheritable_acl(&parent.file, parent_path(path))?; @@ -152,7 +152,7 @@ pub(crate) fn create_dir(parent: &Dir, path: &Utf8Path, name: &str, rule: &Rule, // search bit, because the open restores the owner's access first. // The umask is process-wide, so a test can't set one without racing every other test. // When changing this, keep restoring the access of a directory created here before giving up on its open. - let file = open(created || existing == Existing::Repair)? + let file = open(created || existing != Existing::Check)? .ok_or_else(|| Error::io(path, "open directory", io::ErrorKind::NotFound.into()))?; (file, created) } @@ -167,6 +167,7 @@ pub(crate) fn create_dir(parent: &Dir, path: &Utf8Path, name: &str, rule: &Rule, match existing { Existing::Check => check_dir(&dir, path, rule)?, Existing::Repair => repair_dir(&dir, path, rule)?, + Existing::RepairAccess => repair_item(&dir.file, path, rule, Kind::Directory, OwnerChange::Refuse)?, } } // UNTESTED: the new directory and its parent are synced before the directory is returned. @@ -184,18 +185,30 @@ pub(crate) fn create_dir(parent: &Dir, path: &Utf8Path, name: &str, rule: &Rule, } /// Gives the current user back access to the directory `name` of `parent`, which it owns but couldn't open, so that it -/// can be repaired or checked; fails with `denied` when `name` is not such a directory. +/// can be repaired or checked; fails with `denied` when `name` is not such a directory, or with an owner refusal under +/// [`Existing::RepairAccess`] when it is a directory owned by an account `rule` doesn't allow. /// /// The change happens before the volume check that the repair or check makes once the directory is open. /// It is harmless there: mode 0700 gives no account but the owner any access, and the owner could change the mode /// itself. -fn restore_owner_access(parent: &Dir, name: &CStr, rule: &Rule, denied: Error) -> Result<()> { +fn restore_owner_access(parent: &Dir, name: &CStr, rule: &Rule, existing: Existing, denied: Error) -> Result<()> { let euid = effective_uid(); match stat_at(&parent.file, name) { Ok(stat) if entry_kind(stat.st_mode) == EntryKind::Directory && stat.st_uid == euid && owner_allowed(rule, euid, euid) => {} + // UNTESTED: a repair of access only reports a directory it can't open, owned by an account the rule doesn't + // allow, as an owner refusal. + // Only root can give a test directory another owner, and root opens every directory, so no test reaches this. + // When changing this, keep it ahead of any change, so that such a directory is refused as it is. + Ok(stat) + if existing == Existing::RepairAccess + && entry_kind(stat.st_mode) == EntryKind::Directory + && !owner_allowed(rule, stat.st_uid, euid) => + { + return Err(Error::refused(denied.path(), Refusal::Owner)); + } _ => return Err(denied), } check_rule(denied.path(), rule, euid, euid, Kind::Directory)?; @@ -416,15 +429,24 @@ pub(crate) fn check_dir(dir: &Dir, path: &Utf8Path, rule: &Rule) -> Result<()> { } pub(crate) fn repair_dir(dir: &Dir, path: &Utf8Path, rule: &Rule) -> Result<()> { - repair_item(&dir.file, path, rule, Kind::Directory) + repair_item(&dir.file, path, rule, Kind::Directory, OwnerChange::Allow) } pub(crate) fn repair_file(file: &File, path: &Utf8Path, rule: &Rule) -> Result<()> { - repair_item(file, path, rule, Kind::File) + repair_item(file, path, rule, Kind::File, OwnerChange::Allow) +} + +/// Whether a repair may give an item whose owner its rule doesn't allow to the current user. +#[derive(Clone, Copy, PartialEq, Eq)] +enum OwnerChange { + Allow, + Refuse, } /// Brings the owner, mode and ACLs of the open directory or file `file` into line with `rule`, then checks it. -fn repair_item(file: &File, path: &Utf8Path, rule: &Rule, kind: Kind) -> Result<()> { +/// +/// Under [`OwnerChange::Refuse`], an owner the rule doesn't allow is refused before anything changes. +fn repair_item(file: &File, path: &Utf8Path, rule: &Rule, kind: Kind, owner_change: OwnerChange) -> Result<()> { let euid = effective_uid(); let metadata = file .metadata() @@ -440,8 +462,10 @@ fn repair_item(file: &File, path: &Utf8Path, rule: &Rule, kind: Kind) -> Result< os::check_volume(file, path)?; let owner = if owner_allowed(rule, metadata.uid(), euid) { metadata.uid() - } else { + } else if owner_change == OwnerChange::Allow { euid + } else { + return Err(Error::refused(path, Refusal::Owner)); }; check_rule(path, rule, owner, euid, kind)?; let has_extended_acl = match os::check_no_extended_acl(file, path, kind == Kind::Directory) { diff --git a/crates/private-fs/src/windows/mod.rs b/crates/private-fs/src/windows/mod.rs index 9d7a252c0..36bfea69a 100644 --- a/crates/private-fs/src/windows/mod.rs +++ b/crates/private-fs/src/windows/mod.rs @@ -184,7 +184,7 @@ pub(crate) fn create_dir(parent: &Dir, path: &Utf8Path, name: &str, rule: &Rule, check_dir(&dir, path, rule)?; Ok(dir) } - Existing::Repair => { + Existing::Repair | Existing::RepairAccess => { // A directory whose DACL no longer grants the current user anything is still repairable by its owner, // so it is opened with what reading its owner and DACL needs, within the owner's implicit access, and // opened normally once repaired; the repair itself asks for WRITE_DAC only when it writes. @@ -199,7 +199,12 @@ pub(crate) fn create_dir(parent: &Dir, path: &Utf8Path, name: &str, rule: &Rule, let repairing = nt::create(parent.handle(), name, &request) .map_err(|error| Error::io(path, "open directory for repair", error))?; let policy = Policy::new(rule, Kind::Directory, path)?; - repair(repairing.as_handle(), path, Kind::Directory, &policy)?; + let owner_change = if existing == Existing::Repair { + OwnerChange::Allow + } else { + OwnerChange::Refuse + }; + repair(repairing.as_handle(), path, Kind::Directory, &policy, owner_change)?; let handle = reopen(repairing.as_handle(), DIRECTORY_ACCESS) .map_err(|error| Error::io(path, "open directory", error))?; check_item(handle.as_handle(), path, Kind::Directory, &policy.exact())?; @@ -481,20 +486,35 @@ pub(crate) fn check_dir(dir: &Dir, path: &Utf8Path, rule: &Rule) -> Result<()> { pub(crate) fn repair_dir(dir: &Dir, path: &Utf8Path, rule: &Rule) -> Result<()> { let policy = Policy::new(rule, Kind::Directory, path)?; - repair(dir.handle(), path, Kind::Directory, &policy)?; + repair(dir.handle(), path, Kind::Directory, &policy, OwnerChange::Allow)?; check_item(dir.handle(), path, Kind::Directory, &policy.exact()) } /// Brings an open file's owner and DACL into line with `rule`, then checks it exactly. pub(crate) fn repair_file(file: &File, path: &Utf8Path, rule: &Rule) -> Result<()> { let policy = Policy::new(rule, Kind::File, path)?; - repair(file.as_handle(), path, Kind::File, &policy)?; + repair(file.as_handle(), path, Kind::File, &policy, OwnerChange::Allow)?; check_item(file.as_handle(), path, Kind::File, &policy.exact()) } +/// Whether a repair may give an item whose owner its rule doesn't allow to the current user. +#[derive(Clone, Copy, PartialEq, Eq)] +enum OwnerChange { + Allow, + Refuse, +} + /// Brings the owner and DACL of the item `handle` refers to into line with `policy`, through a handle opened again /// with the repair's access only, without checking the result. -fn repair(handle: BorrowedHandle<'_>, path: &Utf8Path, kind: Kind, policy: &Policy) -> Result<()> { +/// +/// Under [`OwnerChange::Refuse`], an owner the rule doesn't allow, or a missing one, is refused before anything changes. +fn repair( + handle: BorrowedHandle<'_>, + path: &Utf8Path, + kind: Kind, + policy: &Policy, + owner_change: OwnerChange, +) -> Result<()> { classify(handle, path, kind)?; let security = OwnerAndDacl::of_handle(handle).map_err(|error| Error::io(path, "read access control list", error))?; @@ -511,7 +531,7 @@ fn repair(handle: BorrowedHandle<'_>, path: &Utf8Path, kind: Kind, policy: &Poli let owner = match current_owner { Some(owner) if owner_allowed => owner.clone(), // The repair would make the current user the owner, which the rule must then allow. - _ if policy.allows_user_owner() => policy.accounts.user.clone(), + _ if owner_change == OwnerChange::Allow && policy.allows_user_owner() => policy.accounts.user.clone(), _ => return Err(Error::refused(path, Refusal::Owner)), }; let descriptor = Descriptor::new(&owner, &policy.dacl(&owner)) diff --git a/devolutions-agent/src/identity/INTENT.md b/devolutions-agent/src/identity/INTENT.md index 07f453717..aebdf8585 100644 --- a/devolutions-agent/src/identity/INTENT.md +++ b/devolutions-agent/src/identity/INTENT.md @@ -3,10 +3,10 @@ The agent's generic command-line and configuration modules only route to them, s ## CLI -- The only command added is `identity enroll`, with the token as its argument or `-`. +- The only command added is `identity enroll -`, which reads the token from the first line of standard input; a token kept in a file is passed by redirecting standard input from it. It validates the token format, writes a pending-enrollment file, which the service then consumes, prints the file's path and exits with status 0. For an invalid token or a write failure, it does not report success and exits with a nonzero status. -- `identity enroll -` reads the token from the first line of standard input, so that the token never appears in a process listing or an installer log. +- The command never takes the token as an argument, so that a token never appears in a process listing, shell history or an installer log; any other argument is refused without being echoed. - On Windows, an elevated administrator submits through the service's drop box; until the service has provisioned it, the command fails with an error asking to start the service once, and never creates the directory itself. - The existing `enroll` command belongs to Agent Tunnel and does not change. - The CLI does not grow beyond what an end user reasonably needs. @@ -26,6 +26,7 @@ The agent's generic command-line and configuration modules only route to them, s - The extra trusted root and the access grant for the current user exist only for tests; they are never a production trust or custody path. - The knobs, which the conformance tester relies on: - `renewal_after_secs`: replaces the scheduled renewal time with the specified number of seconds after the certificate's anchor (`crates/agent-identity/INTENT.md`, "Schedules"). + - `min_renewal_interval_secs`: replaces the 10-minute wait between renewals with the specified number of seconds; 0 removes the wait (`crates/agent-identity/INTENT.md`, "Schedules"). - `disable_jitter`: disables renewal, check-in and retry jitter. - `backoff_max_secs`: caps identity-operation retry backoff, including `Retry-After`, at the specified number of seconds. - `pending_poll_interval_ms`: replaces the default 5 s pending-directory poll interval. diff --git a/devolutions-agent/src/identity/channel.rs b/devolutions-agent/src/identity/channel.rs index 14d8ee2e3..b82aa1133 100644 --- a/devolutions-agent/src/identity/channel.rs +++ b/devolutions-agent/src/identity/channel.rs @@ -7,7 +7,7 @@ use std::collections::BTreeSet; use std::sync::Arc; use std::time::{Duration, Instant}; -use agent_channel_client::{Channel, Closure, ClosureClass, Event, TerminalCode}; +use agent_channel_client::{Channel, Closure, ClosureClass, ErrorCode, Event, TerminalCode}; use agent_identity::client; use parking_lot::Mutex; use serde_json::Value; @@ -587,7 +587,7 @@ impl Links { if was_authenticated { warn!( grpc_code = ?closure.grpc_code, - error_code = closure.error_code.as_deref(), + error_code = closure.error_code.as_ref().map(ErrorCode::as_str), reason = %closure.reason, ?reopen_after, "Agent channel stream closed" @@ -596,7 +596,7 @@ impl Links { } else { debug!( grpc_code = ?closure.grpc_code, - error_code = closure.error_code.as_deref(), + error_code = closure.error_code.as_ref().map(ErrorCode::as_str), reason = %closure.reason, ?reopen_after, "Agent channel stream did not open" diff --git a/devolutions-agent/src/identity/cli.rs b/devolutions-agent/src/identity/cli.rs index 14995adc7..c0b2d7eec 100644 --- a/devolutions-agent/src/identity/cli.rs +++ b/devolutions-agent/src/identity/cli.rs @@ -25,15 +25,8 @@ fn enroll(args: &mut dyn Iterator) -> anyhow::Result<()> { if subcommand != "enroll" { bail!("unknown identity subcommand"); } - let raw = Zeroizing::new(args.next().context("missing identity enrollment token")?); - if args.next().is_some() { - bail!("identity enroll accepts one token"); - } - let raw = if raw.as_str() == "-" { - read_token(io::stdin().lock())? - } else { - raw - }; + require_stdin_argument(args)?; + let raw = read_token(io::stdin().lock())?; let token = Token::parse(&raw)?; let conf = ConfHandle::init()?.get_conf(); let grant_current_user = conf @@ -47,6 +40,20 @@ fn enroll(args: &mut dyn Iterator) -> anyhow::Result<()> { Ok(()) } +/// Accepts only the `-` that follows `identity enroll`, so that a token never appears in a process listing or shell +/// history. +/// +/// A refused argument may be a token, so the error never includes it, and it is zeroized when dropped. +fn require_stdin_argument(args: &mut dyn Iterator) -> anyhow::Result<()> { + let argument = args.next().map(Zeroizing::new); + let extra = args.next().map(Zeroizing::new); + match (argument.as_ref().map(|argument| argument.as_str()), extra) { + (Some("-"), None) => Ok(()), + (None, _) => bail!("missing `-`: identity enroll reads the enrollment token from standard input"), + _ => bail!("identity enroll accepts only `-` and reads the enrollment token from standard input"), + } +} + fn read_token(mut input: R) -> anyhow::Result> { let mut line = Zeroizing::new(String::new()); if input.read_line(&mut line).context("read enrollment token from stdin")? == 0 { @@ -61,8 +68,41 @@ fn read_token(mut input: R) -> anyhow::Result> { #[cfg(test)] mod tests { + use rstest::rstest; + use super::*; + const TOKEN: &str = "dvaet1.bag.secret"; + + fn check(args: &[&str]) -> anyhow::Result<()> { + require_stdin_argument(&mut args.iter().map(|arg| (*arg).to_owned())) + } + + // oracle: intent devolutions-agent/src/identity/INTENT.md §CLI + #[test] + fn identity_enroll_accepts_hyphen_alone() { + check(&["-"]).expect("accept `-`"); + } + + // oracle: intent devolutions-agent/src/identity/INTENT.md §CLI + #[test] + fn identity_enroll_without_argument_asks_for_hyphen() { + let error = check(&[]).expect_err("refuse a missing `-`"); + assert!(format!("{error:#}").contains("`-`"), "{error:#}"); + } + + // oracle: intent devolutions-agent/src/identity/INTENT.md §CLI + #[rstest] + #[case::token(&[TOKEN])] + #[case::token_after_hyphen(&["-", TOKEN])] + #[case::token_before_hyphen(&[TOKEN, "-"])] + #[case::token_option(&["--token", TOKEN])] + fn identity_enroll_refuses_token_argument_without_echoing_it(#[case] args: &[&str]) { + let error = format!("{:#}", check(args).expect_err("refuse a token argument")); + assert!(!error.contains(TOKEN), "{error}"); + assert!(error.contains("standard input"), "{error}"); + } + // oracle: intent devolutions-agent/src/identity/INTENT.md §CLI #[test] fn stdin_identity_token_trims_whitespace_and_rejects_empty_input() { diff --git a/devolutions-agent/src/identity/config.rs b/devolutions-agent/src/identity/config.rs index 8956fa7be..1bfa0fb53 100644 --- a/devolutions-agent/src/identity/config.rs +++ b/devolutions-agent/src/identity/config.rs @@ -49,6 +49,8 @@ pub struct IdentityDebugConf { #[serde(skip_serializing_if = "Option::is_none")] pub renewal_after_secs: Option, #[serde(skip_serializing_if = "Option::is_none")] + pub min_renewal_interval_secs: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub disable_jitter: Option, #[serde(skip_serializing_if = "Option::is_none")] pub extra_trusted_root: Option, @@ -112,6 +114,7 @@ mod tests { "__debug__": { "identity": { "renewal_after_secs": 1, + "min_renewal_interval_secs": 5, "disable_jitter": false, "extra_trusted_root": "ca.pem", "backoff_max_secs": 2, @@ -134,6 +137,7 @@ mod tests { .context("missing identity debug config")?; assert!(debug.has_settings()); assert_eq!(debug.renewal_after_secs, Some(1)); + assert_eq!(debug.min_renewal_interval_secs, Some(5)); assert_eq!(debug.disable_jitter, Some(false)); assert_eq!(debug.extra_trusted_root.as_deref(), Some(Utf8Path::new("ca.pem"))); assert_eq!(debug.backoff_max_secs, Some(2)); diff --git a/devolutions-agent/src/identity/mod.rs b/devolutions-agent/src/identity/mod.rs index d175aef20..60585e531 100644 --- a/devolutions-agent/src/identity/mod.rs +++ b/devolutions-agent/src/identity/mod.rs @@ -13,8 +13,8 @@ use agent_channel_client::{Backoff, ChannelClient, uniform_jitter}; use agent_identity::client::{Client, ClientError, PermanentCode, TerminalCode}; use agent_identity::pending::{self, PendingRead}; use agent_identity::state::{ - EnrolledIdentity, EnrollmentDiscardReason, IDENTITY_KEY_PREFIX, KeyRecord, KeySlots, PendingDiscard, Store, - StoredIdentity, UntrustedStoredIdentity, + EnrolledIdentity, EnrollmentDiscardReason, ForeignIdentityDirectory, IDENTITY_KEY_PREFIX, KeyRecord, KeySlots, + PendingDiscard, Store, StoredIdentity, UntrustedStoredIdentity, }; use agent_identity::token::Token; use agent_identity::{csr, metadata}; @@ -193,8 +193,8 @@ async fn run_identity( /// Why pending enrollment stopped. enum Stopped { Shutdown, - /// An authority directory or `identity.json` failed its check, which stops all identity processing until it is - /// fixed. + /// An identity directory has an owner the agent does not accept, or an authority directory or `identity.json` failed + /// its check, which stops all identity processing until it is fixed. Untrusted, } @@ -256,7 +256,7 @@ enum PendingOutcome { Retry { retry_after: Option }, } -#[derive(Default)] +#[derive(Clone, Default)] struct RetryState { failures: u32, next_attempt: Option, @@ -316,29 +316,38 @@ async fn run_pending_enrollments( let mut last_check_error = None; let mut last_list_error = None; loop { - // `crates/agent-identity/INTENT.md` ("Local state"): an authority directory or `identity.json` that fails its - // check stops all identity processing; start-up reports it and waits until it is fixed. - // Any other error only kept the check from running, so the poll retries it rather than stop every runner and its - // streams. + // `crates/agent-identity/INTENT.md` ("Local state"): an identity directory with an owner the agent does not + // accept, or an authority directory or `identity.json` that fails its check, stops all identity processing; + // start-up reports it and waits until it is fixed. + // The poll runs the checks a state change runs before it writes, so that it notices either without one. + // Like start-up, this also resets wrong permissions on the identity directories. + // Any other error only kept the checks from running, so the poll retries them rather than stop every runner and + // its streams. // UNTESTED: both outcomes are logged with the error, because stopping every identity is otherwise unexplained // and a check that keeps failing otherwise holds pending enrollment silently. // No test captures service logs; when changing this, keep the error in both logs. - if let Err(error) = store.check_stored_identities() { - if let Some(untrusted) = error - .chain() - .find_map(|cause| cause.downcast_ref::()) - { + if let Err(error) = store.check_state() { + if let Some(path) = error.chain().find_map(|cause| { + cause + .downcast_ref::() + .map(|untrusted| &untrusted.path) + .or_else(|| { + cause + .downcast_ref::() + .map(|foreign| &foreign.path) + }) + }) { warn!( error = format!("{error:#}"), - path = %untrusted.path, - "Stopping every Agent Identity because a stored identity failed its check" + %path, + "Stopping every Agent Identity because the identity state failed its check" ); return Stopped::Untrusted; } if log_is_due(&mut last_check_error) { warn!( error = format!("{error:#}"), - "Cannot check the stored Agent Identities; retrying at the next poll" + "Cannot check the Agent Identity state; retrying at the next poll" ); } if wait_or_stop(shutdown, poll_interval).await { @@ -517,6 +526,8 @@ async fn enroll_pending( #[cfg(test)] mod tests { + use std::io::Write as _; + use super::*; #[cfg(not(windows))] @@ -749,14 +760,132 @@ mod tests { Ok(()) } + #[rstest::rstest] + #[case::identity("identity")] + #[case::authorities("identity/authorities")] + #[case::keys("identity/keys")] + #[case::rejected("identity/rejected")] + #[case::pending("identity/pending")] + #[tokio::test] + async fn an_identity_directory_given_to_another_account_after_start_up_stops_identity_processing( + #[case] name: &str, + ) -> anyhow::Result<()> { + // oracle: intent crates/agent-identity/INTENT.md §Local state + // An identity directory owned by another account stops all identity processing until it is fixed, also when it + // changes owner after start-up; the caller then stops every runner and its streams, and start-up resumes without + // a restart once the owner is fixed. + let data_dir = TestDataDir::new()?; + let shared = test_shared(&data_dir)?; + let (_shutdown_handle, mut shutdown) = devolutions_gateway_task::ShutdownHandle::new(); + let mut runners = Runners::new(Arc::clone(&shared), shutdown.clone()); + let path = data_dir.0.join(name); + if !give_to_another_account(&path)? { + let needs = if cfg!(unix) { "root" } else { "an elevated token" }; + writeln!( + std::io::stderr(), + "SKIP: an_identity_directory_given_to_another_account_after_start_up_stops_identity_processing needs \ + {needs} to give a directory another owner" + )?; + return Ok(()); + } + + let stopped = tokio::time::timeout( + Duration::from_secs(10), + run_pending_enrollments( + &shared, + &mut runners, + &data_dir.0, + Duration::from_millis(10), + &mut shutdown, + ), + ) + .await; + let start_up_while_foreign = data_dir.store().map(drop); + give_back_to_the_agent(&path)?; + let start_up_once_fixed = data_dir.store().map(drop); + + assert!( + matches!(stopped.context("pending poll kept running")?, Stopped::Untrusted), + "the pending poll stopped for another reason" + ); + assert!( + start_up_while_foreign.is_err(), + "start-up accepted an identity directory owned by another account" + ); + start_up_once_fixed.context("start-up did not resume once the identity directory was fixed")?; + Ok(()) + } + + /// Gives `path` to an account other than the agent's, or returns `false` when this process can't. + fn give_to_another_account(path: &Utf8Path) -> anyhow::Result { + #[cfg(unix)] + { + // SAFETY: Reading the process's effective UID has no preconditions. + if unsafe { libc::geteuid() } != 0 { + return Ok(false); + } + // The conventional `nobody` account, which is neither root nor the agent's account here. + std::os::unix::fs::chown(path, Some(65534), None)?; + Ok(true) + } + #[cfg(windows)] + { + // BUILTIN\Administrators, which only an elevated token can make the owner. + set_owner(path, "*S-1-5-32-544") + } + } + + /// Gives `path` back to the account the test runs as, which is the agent's. + fn give_back_to_the_agent(path: &Utf8Path) -> anyhow::Result<()> { + #[cfg(unix)] + { + // SAFETY: Reading the process's effective UID has no preconditions. + std::os::unix::fs::chown(path, Some(unsafe { libc::geteuid() }), None)?; + } + #[cfg(windows)] + { + // The SID, because icacls cannot map the name of an Entra ID or Microsoft account. + // `whoami /user /fo csv /nh` prints `"",""`; the name may not be UTF-8 in the console code page. + let output = std::process::Command::new("whoami") + .args(["/user", "/fo", "csv", "/nh"]) + .output()?; + anyhow::ensure!(output.status.success(), "whoami failed"); + let stdout = String::from_utf8_lossy(&output.stdout); + let sid = stdout + .trim() + .rsplit(',') + .next() + .map(|field| field.trim_matches('"')) + .filter(|sid| sid.starts_with("S-1-")) + .with_context(|| format!("no SID in whoami output: {stdout}"))?; + let owner = format!("*{sid}"); + anyhow::ensure!(set_owner(path, &owner)?, "cannot give {path} back to {owner}"); + } + Ok(()) + } + + /// Makes `owner`, an account name or `*` followed by a SID, the owner of `path`, and reports whether `icacls` could. + #[cfg(windows)] + fn set_owner(path: &Utf8Path, owner: &str) -> anyhow::Result { + let status = std::process::Command::new("icacls") + .arg(path) + .arg("/setowner") + .arg(owner) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status()?; + Ok(status.success()) + } + #[tokio::test] async fn a_check_that_cannot_run_neither_stops_the_pending_poll_nor_holds_enrollment_once_it_runs_again() -> anyhow::Result<()> { // oracle: intent crates/agent-identity/INTENT.md §Local state - // Only an authority directory or `identity.json` that fails the check stops all identity processing. - // An error that keeps the check from running, here listing the authority directories, is not such a failure, - // so the poll keeps running, which keeps its caller from stopping every runner and its streams, and processes - // pending files again once the check runs. + // Only an identity directory with an owner the agent does not accept, or an authority directory or + // `identity.json` that fails the check, stops all identity processing. + // An error that keeps the checks from running, here a file in place of the authority directories, is not such a + // failure, so the poll keeps running, which keeps its caller from stopping every runner and its streams, and + // processes pending files again once the checks run. let data_dir = TestDataDir::new()?; let shared = test_shared(&data_dir)?; let (_shutdown_handle, mut shutdown) = devolutions_gateway_task::ShutdownHandle::new(); @@ -767,12 +896,15 @@ mod tests { let authorities = data_dir.0.join("identity").join("authorities"); std::fs::remove_dir(&authorities)?; std::fs::write(&authorities, b"")?; - let check = shared.store.check_stored_identities(); + let check = shared.store.check_state(); assert!( - check - .as_ref() - .is_err_and(|error| !error.chain().any(|cause| cause.is::())), - "listing a file as the authority directories must fail without an untrusted identity: {check:?}" + check.as_ref().is_err_and(|error| { + !error + .chain() + .any(|cause| cause.is::() || cause.is::()) + }), + "a file in place of the authority directories must fail the checks without an untrusted identity or a \ + foreign identity directory: {check:?}" ); let poll = run_pending_enrollments( diff --git a/devolutions-agent/src/identity/runner.rs b/devolutions-agent/src/identity/runner.rs index a6bb2b6c2..9b78ffc91 100644 --- a/devolutions-agent/src/identity/runner.rs +++ b/devolutions-agent/src/identity/runner.rs @@ -26,6 +26,10 @@ const CHECK_IN_INTERVAL: Duration = Duration::from_secs(24 * 60 * 60); const CHANNEL_FAILURE_CHECK_IN_AFTER: Duration = Duration::from_secs(60 * 60); /// Bounds every wait, so that a wall-clock change delays a scheduled action by at most this long. const MAX_WAIT: Duration = Duration::from_secs(60); +/// The least time from a completed renewal to the start of the next, so that the agent never renews in a tight loop. +/// +/// `__debug__.identity.min_renewal_interval_secs` replaces it. +const MIN_RENEWAL_INTERVAL: Duration = Duration::from_secs(10 * 60); /// The state and services every runner shares with pending enrollment. pub(super) struct Shared { @@ -83,10 +87,14 @@ struct Runner { /// Backoff after a local failure, such as a busy state lock. local_retry: RetryState, renewal_requested: bool, + /// When the last renewal of this run was confirmed. + last_renewal: Option, /// A terminal outcome the authority returned, recorded before any other action. terminal: Option<&'static str>, /// Whether pending-key recovery signs with the current key after the expired pending key drew `identity_unknown`. recovery_signs_with_current: bool, + /// The pending-key recoveries started since the last confirmed renewal of this run. + recoveries: Recoveries, /// When this run stored or loaded each certificate, which anchors its renewal schedule. load_times: LoadTimes, /// The renewal jitter sample of the current key, drawn once for each key. @@ -118,8 +126,10 @@ impl Runner { check_in_retry: RetryState::default(), local_retry: RetryState::default(), renewal_requested: false, + last_renewal: None, terminal: None, recovery_signs_with_current: false, + recoveries: Recoveries::default(), load_times: LoadTimes::default(), renewal_jitter: None, next_check_in: None, @@ -256,11 +266,15 @@ impl Runner { return Ok(Some(Instant::now())); } - let renewal_at = if self.renewal_requested { + let due = if self.renewal_requested { now } else { self.renewal_at(&identity, now)? }; + // UNTESTED: a successful `confirm` records `last_renewal`, and every new renewal, scheduled or requested, starts + // through `renewal_start`; `renewal_start` is tested, this wiring isn't. + // When changing it, keep both on this path, so that the minimum interval holds for every renewal. + let renewal_at = renewal_start(&self.shared.settings, due, self.last_renewal); let renewal_wake = later(renewal_at, self.renewal_retry.next_attempt); if renewal_wake <= now { self.renew(&mut identity, false).await?; @@ -397,7 +411,9 @@ impl Runner { } store.promote_pending(identity)?; self.renewal_retry.succeeded(); + self.recoveries.confirmed(); self.renewal_requested = false; + self.last_renewal = Some(Instant::now()); self.confirm_unknown = false; self.channel_after_confirm = false; if std::mem::take(&mut self.check_in_after_confirm) { @@ -421,7 +437,12 @@ impl Runner { "Agent Identity pending certificate expired before confirmation; recovering" ); store.begin_recovery(identity)?; - self.renewal_retry.succeeded(); + // UNTESTED: each recovery waits the backoff of `recoveries`, which only a successful `confirm` resets + // and a successful renew leaves alone; `Recoveries` is tested, this wiring isn't. + // An authority whose every pending certificate is already expired at `confirm` would otherwise keep + // the agent renewing in a tight loop, generating a key each time. + // When changing this, start every recovery through `recoveries` and reset it on `confirm` only. + self.renewal_retry = self.recoveries.start(&self.shared.settings); self.recovery_signs_with_current = false; } Err(ClientError::Terminal(code)) => self.reject(code.as_str()), @@ -459,7 +480,8 @@ impl Runner { self.next_check_in = Some(Instant::now() + self.check_in_interval()); } Err(ClientError::CertificateExpired) => { - // `CONTRACT.agent-identity.md` §5.4: renew at once, and check in again only after `confirm`. + // `CONTRACT.agent-identity.md` §5.4: renew, no sooner than the minimum renewal interval after the + // previous renewal, and check in again only after `confirm`. self.renewal_requested = true; self.check_in_after_confirm = true; self.next_check_in = None; @@ -490,8 +512,9 @@ impl Runner { Notice::Terminal(code) => self.reject(code.as_str()), Notice::CertificateExpired { key_name } => { if key_name == identity.keys.current.key_name { - // `CONTRACT.agent-identity.md` §5.4: renew at once, signing with the expired certificate, and - // connect again only after `confirm`. + // `CONTRACT.agent-identity.md` §5.4: renew, no sooner than the minimum renewal interval after + // the previous renewal, signing with the expired certificate, and connect again only after + // `confirm`. warn!( authority_id = %self.authority_id, "Agent Identity certificate expired; renewing before the agent channel reconnects" @@ -750,6 +773,46 @@ impl LoadTimes { } } +/// The backoff of the pending-key recoveries started since the last confirmed renewal, `None` before the first. +/// +/// `CONTRACT.agent-identity.md` §8.1: `confirm` is retried with backoff, and a renew that succeeds during recovery +/// doesn't end that backoff, so that an authority cannot keep the agent renewing in a tight loop +/// (`crates/agent-identity/INTENT.md`, "Schedules"). +#[derive(Default)] +struct Recoveries(Option); + +impl Recoveries { + /// Returns the renewal retry state of a recovery that starts now. + /// + /// The first recovery starts at once, because the expired pending certificate stays within its renewal grace only + /// for its own lifetime; each later one waits a backoff that grows with every recovery. + /// That backoff stops at `MIN_RENEWAL_INTERVAL`, or at `backoff_max_secs` when lower, so that a repeated recovery + /// still renews within the grace of a leaf of the shortest lifetime the authority may set, 15 minutes + /// (`CONTRACT.agent-identity.md` §4), while a loop of recoveries at the cap starts one per `MIN_RENEWAL_INTERVAL`. + fn start(&mut self, settings: &IdentityDebugConf) -> RetryState { + let Some(retry) = &mut self.0 else { + self.0 = Some(RetryState::default()); + return RetryState::default(); + }; + let capped = IdentityDebugConf { + backoff_max_secs: Some( + settings + .backoff_max_secs + .unwrap_or(u64::MAX) + .min(MIN_RENEWAL_INTERVAL.as_secs()), + ), + ..settings.clone() + }; + retry.failed(&capped, None); + retry.clone() + } + + /// Records a confirmed renewal, after which a recovery starts at once again. + fn confirmed(&mut self) { + self.0 = None; + } +} + fn certificate_chain(record: &KeyRecord) -> anyhow::Result<&[String]> { record .certificate_chain @@ -762,6 +825,29 @@ pub(super) fn config_revision(config: &Value) -> u64 { config.get("revision").and_then(Value::as_u64).unwrap_or_default() } +/// Returns when a new renewal due at `due` starts, given when the last renewal of this run was confirmed. +/// +/// `crates/agent-identity/INTENT.md` ("Schedules"): a renewal starts no sooner than `MIN_RENEWAL_INTERVAL` after the +/// previous one completed, except under `renewal_after_secs`. +/// `devolutions-agent/src/identity/INTENT.md` ("Configuration"): `min_renewal_interval_secs` replaces that interval. +fn renewal_start(settings: &IdentityDebugConf, due: Instant, last_renewal: Option) -> Instant { + let Some(last_renewal) = last_renewal else { + return due; + }; + if settings.renewal_after_secs.is_some() { + return due; + } + let min_interval = settings + .min_renewal_interval_secs + .map_or(MIN_RENEWAL_INTERVAL, Duration::from_secs); + // An interval too long for `Instant` to represent is halved until it fits, so that a huge value still holds the + // renewal back for the rest of the run; halving ends at zero, which always fits. + let floor = std::iter::successors(Some(min_interval), |interval| Some(*interval / 2)) + .find_map(|interval| last_renewal.checked_add(interval)) + .unwrap_or(last_renewal); + due.max(floor) +} + fn later(at: Instant, retry_at: Option) -> Instant { retry_at.map_or(at, |retry_at| retry_at.max(at)) } @@ -780,6 +866,8 @@ fn instant_at(at: SystemTime, now: Instant) -> Instant { mod tests { use std::time::UNIX_EPOCH; + use rstest::rstest; + use super::*; #[test] @@ -803,4 +891,132 @@ mod tests { stored_at ); } + + #[test] + fn a_renewal_due_right_after_another_waits_ten_minutes() { + // oracle: intent crates/agent-identity/INTENT.md §Schedules + // A renewal starts no sooner than 10 minutes after the previous renewal of the run completed, whether the + // schedule or the authority asks for it. + let completed = Instant::now(); + let due = completed + Duration::from_secs(1); + assert_eq!( + renewal_start(&IdentityDebugConf::default(), due, Some(completed)), + completed + Duration::from_secs(10 * 60) + ); + } + + // oracle: intent devolutions-agent/src/identity/INTENT.md §Configuration + #[rstest] + #[case::shorter_interval(5, 5)] + #[case::no_interval(0, 1)] + fn min_renewal_interval_secs_replaces_the_floor(#[case] min_interval_secs: u64, #[case] start_after_secs: u64) { + // A renewal due 1 s after the previous one starts `min_renewal_interval_secs` after it, or at once for `0`. + let settings = IdentityDebugConf { + min_renewal_interval_secs: Some(min_interval_secs), + ..IdentityDebugConf::default() + }; + let completed = Instant::now(); + let due = completed + Duration::from_secs(1); + assert_eq!( + renewal_start(&settings, due, Some(completed)), + completed + Duration::from_secs(start_after_secs) + ); + } + + #[test] + fn a_min_renewal_interval_too_long_for_an_instant_holds_renewal_for_the_run() { + // oracle: intent devolutions-agent/src/identity/INTENT.md §Configuration + // `min_renewal_interval_secs` replaces the 10-minute wait, so a value past what an `Instant` can hold still + // keeps the next renewal beyond any run, taken here as a century, rather than falling back to 10 minutes. + let settings = IdentityDebugConf { + min_renewal_interval_secs: Some(u64::MAX), + ..IdentityDebugConf::default() + }; + let completed = Instant::now(); + let due = completed + Duration::from_secs(1); + let century = Duration::from_secs(100 * 365 * 24 * 60 * 60); + assert!(renewal_start(&settings, due, Some(completed)) > completed + century); + } + + // oracle: intent crates/agent-identity/INTENT.md §Schedules + #[rstest] + #[case::due_after_the_floor(true, None, 11 * 60)] + #[case::first_renewal_of_the_run(false, None, 1)] + #[case::renewal_after_secs_set(true, Some(1), 1)] + fn a_renewal_starts_when_it_is_due( + #[case] renewed_before: bool, + #[case] renewal_after_secs: Option, + #[case] due_after_secs: u64, + ) { + // The floor holds a renewal back only until 10 minutes after the previous one, never before the first renewal + // of the run, and never under `renewal_after_secs`. + let settings = IdentityDebugConf { + renewal_after_secs, + ..IdentityDebugConf::default() + }; + let completed = Instant::now(); + let due = completed + Duration::from_secs(due_after_secs); + assert_eq!(renewal_start(&settings, due, renewed_before.then_some(completed)), due); + } + + #[test] + fn a_first_recovery_starts_at_once() { + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §agent-renewal + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §server-certificate-state + // The agent renews when `confirm` answers `certificate_expired`, and the expired pending certificate stays + // within its renewal grace only for its own lifetime, so the first recovery after a confirmed renewal, or in a + // run, adds no wait. + let retry = Recoveries::default().start(&no_jitter()); + assert_eq!(retry.next_attempt, None); + } + + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §agent-renewal + // oracle: intent crates/agent-identity/INTENT.md §Schedules + #[rstest] + #[case::second(2, 2)] + #[case::third(3, 4)] + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §certificates + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §server-certificate-state + // The backoff stops at the 10-minute wait between renewals, so a recovery still renews within the grace of a leaf of + // the shortest lifetime the authority may set, 15 minutes, and `backoff_max_secs` unset doesn't lift that cap. + #[case::capped_at_ten_minutes(20, 10 * 60)] + fn a_recovery_before_a_confirmed_renewal_waits_a_growing_backoff(#[case] recovery: u32, #[case] wait_secs: u64) { + // `confirm` retries with backoff, so a recovery that starts again before a `confirm` succeeds waits `2^n` s + // for the `n`-th consecutive repeat, without jitter, up to 10 minutes. + let settings = no_jitter(); + let mut recoveries = Recoveries::default(); + for _ in 1..recovery { + recoveries.start(&settings); + } + let before = Instant::now(); + let retry = recoveries.start(&settings); + let wait = retry + .next_attempt + .expect("a repeated recovery waits") + .saturating_duration_since(before); + let expected = Duration::from_secs(wait_secs); + assert!( + expected <= wait && wait < expected + Duration::from_secs(1), + "waits {wait:?}" + ); + } + + #[test] + fn a_confirmed_renewal_lets_the_next_recovery_start_at_once() { + // oracle: contract docs/agent-identity/CONTRACT.agent-identity.md §agent-renewal + // A successful `confirm` ends the retries of the renewal it confirms. + let settings = no_jitter(); + let mut recoveries = Recoveries::default(); + recoveries.start(&settings); + recoveries.start(&settings); + recoveries.confirmed(); + assert_eq!(recoveries.start(&settings).next_attempt, None); + } + + fn no_jitter() -> IdentityDebugConf { + IdentityDebugConf { + disable_jitter: Some(true), + ..IdentityDebugConf::default() + } + } } diff --git a/docs/agent-identity/CONTRACT.agent-identity.md b/docs/agent-identity/CONTRACT.agent-identity.md index 71a9235a5..fc6f28a17 100644 --- a/docs/agent-identity/CONTRACT.agent-identity.md +++ b/docs/agent-identity/CONTRACT.agent-identity.md @@ -139,6 +139,7 @@ Unless stated otherwise, implementations must follow these conventions. - The number of CA tiers and the rest of the root profile are each authority's choice, within the rule below. - Every certificate in a chain is signed with `ecdsa-with-SHA256` by a P-256 key or with `ecdsa-with-SHA384` by a P-384 key, never with RSA, and every CA certificate, the root included, carries basicConstraints with `cA` set and key usage with `keyCertSign`. - Leaf: P-256, `CN=`, SAN URI `urn:uuid:`, EKU clientAuth, KeyUsage digitalSignature; its lifetime is a server setting with a default of 90 days, capped at the issuing root's `notAfter`. + The setting is at least 15 minutes, so that the agent's scheduled renewal, at two-thirds of the lifetime, is not held back by its 10-minute wait between renewals. The leaf carries no other extension except an authority key identifier, a subject key identifier and the enrollment URL extension; in particular it carries no basicConstraints, so a leaf can never act as a CA. - The enrollment URL extension, OID `1.3.6.1.4.1.32473.1.1`, non-critical, holds as an `IA5String` the `u` of the token that enrolled the identity, without trailing slashes (§2); a leaf carries it exactly once, and every certificate issued for that identity carries the same value. That OID sits under the enterprise number RFC 5612 reserves for documentation; it is replaced by one under Devolutions' own IANA enterprise number before the feature is released. @@ -279,7 +280,8 @@ Any other `identity_unknown`, including one returned by that retry, is terminal. `certificate_expired`: -- On `connect` or `check-in`: the agent renews immediately (§8), signing with the expired certificate, which renew still accepts within its grace; it retries the operation only after `confirm`. +- On `connect` or `check-in`: the agent renews (§8), signing with the expired certificate, which renew still accepts within its grace; it retries the operation only after `confirm`. + It renews immediately, except that it may wait until 10 minutes after it last completed a renewal of that identity. - On `renew`: the certificate is beyond grace. It is terminal. - On `confirm`: see §8. diff --git a/docs/agent-identity/INTENT.agent-identity.md b/docs/agent-identity/INTENT.agent-identity.md index 43577caf3..516218860 100644 --- a/docs/agent-identity/INTENT.agent-identity.md +++ b/docs/agent-identity/INTENT.agent-identity.md @@ -162,6 +162,7 @@ Not intended: Signing the URL, as DPoP's `htu` does, would make the authority configure its external URL and compare it exactly, and would cost about 95 bytes per request. - The agent supplies each request's freshness, a single-use nonce and its own timestamp within a window, and the authority issues none, so every operation takes one round trip and a lost response never stalls the agent. Authority-issued nonces would tolerate a skewed agent clock, but they cost a round trip after an idle period, stall a retry after a lost response, and need issuance state shared across authority nodes. +- The agent applies an authority's `server_time` to a `clock_skew` retry only when it is within 1 day of the agent's clock; beyond that, the refusal stands as a transient error, so that a wrong or forged time cannot make the agent date a signature far from its own clock. - The authority records a nonce only after the request signature and any body digest are valid. Requests with an invalid signature or body digest therefore cannot consume nonce storage. - The authority keeps each accepted nonce at least until 60 s after its signature expires, the last moment the freshness window could accept the same signature again. diff --git a/docs/agent-identity/README.md b/docs/agent-identity/README.md index 2e3a5fa3a..d3fbf7370 100644 --- a/docs/agent-identity/README.md +++ b/docs/agent-identity/README.md @@ -22,7 +22,6 @@ Get-Content -Raw 'C:\Secure\enrollment-token.txt' | ``` `identity enroll -` reads the token from the first line of standard input, so the token never appears in a process listing or an installer log. -`identity enroll ` also works, but exposes the token on the command line. On Windows, run the command as SYSTEM or from an elevated administrator prompt; any other account is refused. diff --git a/testsuite/tests/private_fs/unix.rs b/testsuite/tests/private_fs/unix.rs index b3f9c1d3f..9e87cbc59 100644 --- a/testsuite/tests/private_fs/unix.rs +++ b/testsuite/tests/private_fs/unix.rs @@ -669,6 +669,51 @@ fn root_only_an_item_owned_by_another_account_is_refused_until_repaired() { assert_eq!(fs::metadata(scratch.path.join("dir")).unwrap().uid(), 0); } +// Catches a repair of access that refuses, rather than repairs, a directory the current user owns. +// oracle: intent crates/private-fs/INTENT.md §paths +#[rstest] +#[case::open_to_others(0o755)] +#[case::closed_to_its_owner(0o000)] +fn a_repair_of_access_restores_the_mode_of_a_directory_the_current_user_owns(#[case] wrong: u32) { + let scratch = Scratch::new(); + let base = scratch.base(); + drop(base.create_dir("dir", &rule(), Existing::Check).unwrap()); + let dir = scratch.path.join("dir"); + set_mode(&dir, wrong); + + base.create_dir("dir", &rule().exact(), Existing::RepairAccess).unwrap(); + + assert_eq!(mode(&dir), 0o700); +} + +// Root can access everything, so only root can give the test a directory that another account owns. +// oracle: intent crates/private-fs/INTENT.md §paths +// oracle: intent crates/agent-identity/INTENT.md §Local state +#[test] +fn root_only_a_repair_of_access_refuses_an_owner_the_rule_does_not_allow_without_changing_it() { + let scratch = Scratch::new(); + if !scratch.as_root() { + skip( + "root_only_a_repair_of_access_refuses_an_owner_the_rule_does_not_allow_without_changing_it", + "needs root", + ); + return; + } + let other = 12345; + let base = scratch.base(); + drop(base.create_dir("dir", &rule(), Existing::Check).unwrap()); + let dir = scratch.path.join("dir"); + // Wrong access as well, so that a repair would show in the mode even if it kept the owner. + set_mode(&dir, 0o755); + std::os::unix::fs::chown(&dir, Some(other), None).unwrap(); + + let error = base.create_dir("dir", &rule(), Existing::RepairAccess).unwrap_err(); + + assert!(matches!(refusal(&error), Some(Refusal::Owner)), "{error:?}"); + let metadata = fs::symlink_metadata(&dir).unwrap(); + assert_eq!((metadata.uid(), metadata.mode() & 0o7777), (other, 0o755)); +} + // private-fs never chooses a broader access than the rule: an owner other than root can always change the mode. // oracle: intent crates/private-fs/INTENT.md §rules #[test] diff --git a/testsuite/tests/private_fs/windows.rs b/testsuite/tests/private_fs/windows.rs index c488468d3..d654a7378 100644 --- a/testsuite/tests/private_fs/windows.rs +++ b/testsuite/tests/private_fs/windows.rs @@ -564,6 +564,114 @@ fn a_repair_takes_ownership_from_an_owner_the_rule_does_not_allow() -> anyhow::R Ok(()) } +// oracle: intent crates/private-fs/INTENT.md §paths +#[rstest] +#[case::extra_account(&[(user(), FULL, INHERITED_BY_CHILDREN), (users(), FULL, INHERITED_BY_CHILDREN)])] +#[case::nothing_for_the_owner(&[(users(), READ, 0)])] +fn a_repair_of_access_restores_the_dacl_of_a_directory_the_current_user_owns( + #[case] dacl: &[AllowEntry], +) -> anyhow::Result<()> { + // Catches a repair of access that refuses, rather than repairs, a directory whose owner the rule allows. + let test = TestDir::new(); + let base = Dir::create_base(test.path(), &user_rule())?; + drop(base.create_dir("child", &user_rule(), Existing::Check)?); + let child = test.path().join("child"); + set_dacl(&child, dacl, InheritableAclKind::Protected)?; + + drop(base.create_dir("child", &user_rule().exact(), Existing::RepairAccess)?); + + assert_eq!(security(&child)?, (user(), vec![(user(), FULL, INHERITED_BY_CHILDREN)])); + Ok(()) +} + +// oracle: intent crates/private-fs/INTENT.md §paths +// oracle: intent crates/agent-identity/INTENT.md §Local state +#[test] +fn a_repair_of_access_refuses_an_owner_the_rule_does_not_allow_without_changing_it() -> anyhow::Result<()> { + // Catches a repair of access that takes ownership, as a repair does, from an owner whose DACL lets the current + // user read and rewrite the owner and DACL but not open the directory for a check. + const READ_ATTRIBUTES: u32 = 0x80; + const WRITE_DAC: u32 = 0x0004_0000; + const WRITE_OWNER: u32 = 0x0008_0000; + let test = TestDir::new(); + let base = Dir::create_base(test.path(), &user_rule())?; + drop(base.create_dir("child", &user_rule(), Existing::Check)?); + let child = test.path().join("child"); + // The owner is given first, so that a skipped test leaves a directory it can remove. + if !set_owner(&child, &well_known(Security::WinLocalServiceSid))? { + skip( + "a_repair_of_access_refuses_an_owner_the_rule_does_not_allow_without_changing_it", + "the process token doesn't hold the restore privilege", + ); + return Ok(()); + } + set_dacl( + &child, + &[(user(), READ_CONTROL | WRITE_DAC | WRITE_OWNER | READ_ATTRIBUTES, 0)], + InheritableAclKind::Protected, + )?; + // An open through `std` adds SYNCHRONIZE, which this DACL withholds, so the test reads as a backup operator. + let read_as_backup = || { + let _backup = BackupPrivilege::enable()?; + security(&child) + }; + let before = read_as_backup()?; + + let error = refusal(base.create_dir("child", &user_rule().exact(), Existing::RepairAccess)); + + assert!( + is_refused(&error, |refusal| matches!(refusal, Refusal::Owner)), + "{error:?}" + ); + assert_eq!(read_as_backup()?, before); + // The DACL is enough for a repair to take the directory, so the refusal above is the repair of access's own. + drop(base.create_dir("child", &user_rule().exact(), Existing::Repair)?); + assert_eq!(security(&child)?.0, user()); + Ok(()) +} + +// oracle: intent crates/private-fs/INTENT.md §paths +// oracle: intent crates/agent-identity/INTENT.md §Local state +#[test] +fn a_repair_of_access_leaves_a_directory_whose_owner_it_cannot_read_unchanged() -> anyhow::Result<()> { + // Catches a repair of access that writes to a directory whose owner it couldn't check. + let test = TestDir::new(); + let base = Dir::create_base(test.path(), &user_rule())?; + drop(base.create_dir("child", &user_rule(), Existing::Check)?); + let child = test.path().join("child"); + let _give_back = GiveBack(&child); + let local_service = well_known(Security::WinLocalServiceSid); + if !set_owner(&child, &local_service)? { + skip( + "a_repair_of_access_leaves_a_directory_whose_owner_it_cannot_read_unchanged", + "the process token doesn't hold the restore privilege", + ); + return Ok(()); + } + // The current user's entry still grants it WRITE_DAC, which this DACL then takes away with every other access. + set_dacl(&child, &[(system(), FULL, 0)], InheritableAclKind::Protected)?; + let read_as_backup = || { + let _backup = BackupPrivilege::enable()?; + security(&child) + }; + + let result = base.create_dir("child", &user_rule().exact(), Existing::RepairAccess); + + assert!(result.is_err()); + assert_eq!(read_as_backup()?, (local_service, vec![(system(), FULL, 0)])); + return Ok(()); + + /// Gives the directory back to the current user however the test ends, so that the test directory can be removed. + struct GiveBack<'a>(&'a Utf8Path); + + impl Drop for GiveBack<'_> { + fn drop(&mut self) { + let _ = set_owner(self.0, &user()); + let _ = set_dacl(self.0, &[(user(), FULL, 0)], InheritableAclKind::Protected); + } + } +} + // oracle: intent crates/private-fs/INTENT.md §vocabulary #[test] fn administrators_as_an_owner_allow_a_member_of_the_group() -> anyhow::Result<()> {