Skip to content

fix(agent): stop identities when a directory changes owner - #2178

Draft
Benoît Cortier (CBenoit) wants to merge 24 commits into
masterfrom
claude/agent-foreign-dir-midrun-046b3f
Draft

Benoît Cortier (CBenoit) wants to merge 24 commits into
masterfrom
claude/agent-foreign-dir-midrun-046b3f

Conversation

@CBenoit

Copy link
Copy Markdown
Member

An identity directory owned by another account stops all Agent Identity processing until it is fixed, because the account that created it can keep a handle that still changes it after a reset. Start-up already refused such a directory, but while the agent ran, a directory that changed owner only made each state change fail: runners kept renewing and checking in, and their agent channel streams stayed open.

The pending-directory poll now provisions the identity directories, as every state change does before it writes, then checks the stored identities, without taking the state lock. A directory whose owner the agent doesn't accept stops every runner and its streams, as an untrusted authority directory or identity.json already does, and the agent resumes without a restart once the owner is fixed. A directory the agent owns with wrong permissions is still repaired and stops nothing; the poll now resets such permissions on each interval, which every state change already did.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

🤖 Generated with Claude Code

A token passed as an argument shows up in process listings and shell
history, so `identity enroll` takes it only from standard input, and a
token kept in a file is passed by redirection.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resetting the owner of an identity directory cannot revoke a handle its
creator opened before the reset, so such a directory stops identity
processing until it is fixed instead of being repaired in place.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…te file

An untrusted rejection marker or in-progress record is treated as absent
and never deleted as cleanup; the agent writing its own file at that
path replaces it, which opens nothing, since whoever wrote it could
delete it anyway.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tificate

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A URL parser removes tab, carriage return and line feed anywhere in a
URL and trims leading and trailing control characters and spaces, so a
token's `u` or a config's `agent_channel_url` holding them was accepted
as a different URL than its text.

Both values are read as text, so the agent now refuses any character
outside printable ASCII before parsing them: a token with such a `u` is
malformed, and such an `agent_channel_url` is treated as absent with a
warning. The ASCII check that only `u` had moves into the shared parser,
so `agent_channel_url` gets it too.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The agent channel classified a closure as terminal or expired from its
`error-code` trailer alone, whatever gRPC status came with it.
The Agent Identity contract pairs each code with one status:
`PERMISSION_DENIED` with `identity_revoked`, and `UNAUTHENTICATED` with
`identity_unknown` or `certificate_expired`.
The channel now ends the identity or renews only on those exact pairs,
as the HTTP client already does, and treats any other pair as a
transient outage.

Likewise, a `clock_skew` refusal adjusts the clock only when it comes
with `UNAUTHENTICATED`, the status of an authentication failure at
opening.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A token passed as an argument shows up in process listings and shell
history, so `identity enroll` now accepts only `-` and reads the token
from standard input. Any other argument is refused, and the error never
repeats it because it may be a token.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The agent's `identity enroll` command reads the token only from standard input and refuses any other argument without echoing it.
The suite now submits every token through `identity enroll -`, and a new test checks that a token passed as an argument, alone or after `-`, is refused with a nonzero status, leaves no pending-enrollment or in-progress file, and appears on neither output stream, while the same token on standard input is accepted.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On a `clock_skew` answer, the agent retried a signed identity request
dated by the `server_time` the answer carried, however far that was
from its own clock.

The agent now retries only when the offset is at most one day either
way; beyond that it returns the `clock_skew` error, a transient
failure, without a second request.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A new renewal of an identity starts no sooner than ten minutes after
the previous renewal of the run was confirmed, so that an authority
cannot keep the agent renewing in a tight loop, for example with
certificates whose lifetime is zero.

The wait applies however the renewal is triggered: by the schedule, by
an authority request, or after `certificate_expired` on a stream or a
check-in. Retries and the confirmation of a renewal already started are
not delayed, and the first renewal of a run is never held back.

A pending-key recovery that starts again before any renewal is
confirmed waits the renewal retry backoff, which grows until a
`confirm` succeeds, up to the ten-minute wait, so that it stays within
the renewal grace of a leaf of at least 15 minutes. The first recovery
still starts at once, because the expired pending certificate stays
within its renewal grace only for its own lifetime.

`renewal_after_secs` keeps its exact schedule for tests, and the new
`__debug__.identity.min_renewal_interval_secs` setting replaces the
wait, so that tests can renew several times in one run.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The agent logged the authority's text as received: the message of the
gRPC status that closes a stream, the `reason` of `RenewRequested` and
`Reconnect`, and the `error-code` trailer.
That text could hold line breaks or terminal escapes, and its length
was unbounded.

- The status message and both reasons are now escaped as Rust's `Debug`
  does, quotes aside, and cut at 256 characters with a `[truncated]`
  marker, without splitting a character.
- An `error-code` trailer is kept only in the form of an error code:
  snake_case ASCII starting with a letter, at most 64 bytes, and not
  starting with the enrollment token prefix, the rule the HTTP client
  already applies.
  Any other value is recorded as invalid, without its text.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the authority refuses a channel opening with `clock_skew`, the
agent signs one retry offset by the authority's `server-time`.
It accepted any offset, however far from its own clock.

The agent now retries only when the authority's time is within one day
of its own; beyond that, the refusal stands and the channel reopens
after its usual backoff, as the HTTP client does.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
At start-up, the agent repaired its identity directories, `identity`,
`authorities`, `keys`, `rejected` and `pending`, whatever their owner,
taking ownership of one another account owned.
That account can keep a handle opened before the repair, which still
changes the directory afterwards, so a reset doesn't make it private.

private-fs gains `Existing::RepairAccess`, which brings an existing
directory's access into line with its rule but refuses an owner the
rule doesn't allow before changing anything, deciding through the same
handle it repairs through.
On Windows, a directory whose owner can't be read is left unchanged too.

Provisioning uses it: a directory with wrong permissions is still
reset, and one owned by another account fails start-up with an error
naming the directory.
Start-up keeps retrying on its usual interval without repairing, so
identity processing stays stopped until an administrator fixes the
owner, then resumes without a restart.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e renewal wait

The agent waits between renewals of one identity in one run, so that an
authority cannot keep it renewing in a tight loop.
`a_renew_transient_failures_retry_without_state_change` renews three times
in one run without `renewal_after_secs`, which that wait would push past
the case's deadline.
The case now sets `__debug__.identity.min_renewal_interval_secs` to 0 in
its agent configuration, so its renewals still follow one another;
every other case keeps the default wait.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the current user on Windows

Some cases write a pending-enrollment file before the agent's first start, so the suite creates the identity and pending directories itself.
Under UAC elevation, a new directory's default owner is Administrators rather than the current user, and the agent refuses an identity directory owned by another account and never repairs it.
On Windows, the suite now creates each missing directory owned by the current user, with the protected DACL it already gives pending files, inherited by what is created inside.
Existing directories, such as those the agent provisioned, keep their owner and DACL.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test withholds SYNCHRONIZE from the directory so that private-fs
can't open it for a check, but reading the directory's security
through `std` asks for SYNCHRONIZE too, so an elevated run failed
before reaching the repair. The test now reads that security under
the backup privilege, as its neighbour does.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…es owner mid-run

An identity directory owned by another account must stop all identity
processing until it is fixed, because the account that created it can
keep a handle that still changes it after a permission reset. Start-up
already refused such a directory, but the pending-directory poll only
checked authority directories and identity.json, so a directory that
changed owner while the agent ran only made each state change fail
while every runner and agent channel stream kept going.

The poll now runs what every state change runs before it writes: it
takes and releases the state lock, whose provisioning refuses an
identity directory with an owner the agent does not accept, then checks
the stored identities. Such a refusal stops every identity and its
streams, as an untrusted authority directory does, and start-up retries
until the directory is fixed, so processing resumes without a restart.
Other errors still only make the poll retry.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The pending-directory poll checks the identity state on every interval
with nothing to write. Taking the state lock for that check made it
wait on any outside handle: on Windows the lock is an unshared handle
on <data-dir>\identity, so Explorer, a shell whose working directory
is that folder, or a backup or antivirus scan made the poll sleep for
30 s on a runtime worker each cycle, and on Unix flock waits without
bound.

The check now provisions the identity directories and checks the
stored identities without locking. Provisioning is what the lock runs
first, so a directory owned by another account is refused exactly as
before, while a sharing violation is an ordinary error the poll
retries.

The test that gives an identity directory to another account now also
covers the authority directories, and gives the directory back by SID,
because icacls cannot map the name of an Entra ID or Microsoft
account.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CBenoit

Copy link
Copy Markdown
Member Author

Evidence and notes

Lines changed (git diff --numstat b43cbd68e e71f43664):

Area Added Removed
Production (agent-identity state.rs, private_file.rs; devolutions-agent identity poll) 45 21
Tests (devolutions-agent/src/identity/mod.rs test module) 132 9
Other 0 0

How it works: the poll calls the new Store::check_state, which runs provision_directories (the same Existing::RepairAccess provisioning start-up and every state change run) and then check_stored_identities. A ForeignIdentityDirectory in the error chain returns the existing Stopped::Untrusted, so run_identity drops every runner and its streams and goes back to start-up, which refuses until the owner is fixed. runner.rs is unchanged.

Tests at e71f43664:

Platform agent-identity devolutions-agent lib devolutions-agent main Owner test
Windows, not elevated 227 passed 201 passed 6 passed skipped (5 cases, needs elevation)
Linux (WSL), normal user 210 passed 135 passed 6 passed skipped (needs root)
Linux (WSL), root, identity::tests only — 12 passed — ran: identity, authorities, keys, rejected, pending

The owner test (an_identity_directory_given_to_another_account_after_start_up_stops_identity_processing) failed on the code before the fix (pending poll kept running) and passes after it, as root on Linux. The CI runners run it only where they are root or elevated; the Windows icacls /setowner path has not run locally.

cargo fmt --check and cargo clippy -p agent-identity -p devolutions-agent --all-targets -- -D warnings are clean on Windows and Linux.

Decisions and side effects worth a look:

  • The poll now resets wrong permissions on the identity directories each interval (default 5 s). Every state change already did this; crates/agent-identity/INTENT.md §Local state names start-up, so the reset is now periodic rather than also per write.
  • The check takes no state lock, so an outside handle on <data-dir>\identity (Explorer, a backup) can't stall the poll. On Windows, while a runner holds the state lock, the poll's provisioning can fail with a sharing violation; that is an ordinary error, retried at the next poll and logged as a warning at most once a minute.
  • At start-up a foreign directory is still logged through the generic "state is unavailable" error, whose text names the directory.
  • The start-up half of the test asserts only that start-up is refused: for identity and keys, the device-key directory check refuses before provisioning, so the error type differs by case.

Note

LLM-assisted content (no human feedback).

@CBenoit

Copy link
Copy Markdown
Member Author

UNTESTED properties

No new UNTESTED: comment. The change relies on one that already exists:

  • devolutions-agent/src/identity/mod.rs, run_identity: dropping the runners aborts every runner task and, with it, its agent channel streams. Tests check that the pending poll returns Stopped::Untrusted, not that a live runner stops.

Note

LLM-assisted content (no human feedback).

Base automatically changed from claude/sec-agent-findings-0f32bb to master October 30, 2026 16:45

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant