Skip to content

fix(agent): harden the Agent Identity client - #2177

Merged
Benoît Cortier (CBenoit) merged 25 commits into
masterfrom
claude/sec-agent-findings-0f32bb
Oct 30, 2026
Merged

Benoît Cortier (CBenoit) merged 25 commits into
masterfrom
claude/sec-agent-findings-0f32bb

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Tightens how the Devolutions Agent handles what an Agent Identity authority sends it, how often it renews, and the state it keeps on disk, and records each rule in the intent files and the contract.

  • Channel closures are classified by the exact pair of gRPC status and error-code the contract defines; any other pair is a transient outage, and only UNAUTHENTICATED with clock_skew carries a clock offset.
  • A clock_skew retry, for signed HTTP requests and for channel openings, applies the authority's time only when it is within one day of the agent's clock, so that a signed request is never dated far from the agent's own time.
  • Authority text in logs (a closing status message, the reason of RenewRequested and Reconnect) is escaped and cut to 256 characters, and an error-code that isn't a valid code is logged as invalid without its text.
  • Base URLs, the token's u and agent_channel_url, refuse white space, control characters and non-ASCII, so that each URL means exactly its text; a malformed agent_channel_url is treated as absent.
  • A renewal starts no sooner than ten minutes after the previous confirmed renewal of the run, however it is triggered, so that an authority cannot keep the agent renewing in a tight loop. A pending-key recovery that starts again before any confirm succeeds waits a growing backoff capped at ten minutes, which keeps it within the renewal grace of a leaf of at least 15 minutes; the contract now requires that minimum. The test-only __debug__.identity.min_renewal_interval_secs replaces the wait.
  • identity enroll reads the token only from standard input (identity enroll -), so that a token never appears in a process listing or shell history; a token argument is refused without being echoed.
  • An identity directory owned by another account is refused rather than repaired, because a handle its creator opened survives an owner reset; a directory the agent owns with wrong permissions is still repaired. private-fs gains Existing::RepairAccess for this.
  • The intent now states that the agent's own write replaces a rejection marker or in-progress record it ignores as untrusted, which is what it already does.
  • The conformance suite submits tokens on standard input, lets its multi-renewal case remove the renewal wait, and on Windows creates the pending directory it prepares owned by the current user.
    A new mock case shortens the wait to 15 seconds and checks that a renewal requested during it starts no sooner than the wait and still starts once it ends.
    Its README takes the short leaves of the DVLS expiry-boundary test from DVLS's test-only lifetime, since the certificate lifetime setting is at least 15 minutes.

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>
@CBenoit

Copy link
Copy Markdown
Member Author

Acceptance evidence (head 8de90d7d2)

Lines changed:

  • Rust under crates/ and devolutions-agent/ (production code with its inline unit tests): +1011 / −94.
  • Conformance suite and testsuite: +396 / −62.
  • Intent, contract and README: +13 / −6.

Windows (unelevated, fresh TEMP):

  • cargo +nightly fmt --check clean.
  • cargo clippy -D warnings clean on private-fs, agent-identity, agent-channel-client, devolutions-agent, agent-identity-conformance and testsuite.
  • Unit tests: agent-channel-client 140, agent-identity 227, agent-identity-conformance 60 (+1 ignored), devolutions-agent 196+6+25.
  • testsuite private_fs: 49.
  • ci/check-oracle-markers.ps1 -Base origin/master exits 0.

Linux (WSL Ubuntu):

  • clippy clean.
  • Unit tests: agent-channel-client 140, agent-identity 210, devolutions-agent 130+6+14.
  • testsuite private_fs: 39.
  • Conformance suite (agent_identity::conformance, file backend): PASS 192, FAIL 0, SKIP 0, N/A 2. Both N/A cases are Windows-only.
  • Three agent-identity-conformance unit tests fail locally with "mode 777, expected 0600". They write under the workspace target/ on a Windows drive mounted in WSL, which ignores chmod, and they fail the same way without this change. CI runs them on a native file system.

Not run locally: the elevated Windows paths (the conformance suite with the Windows software provider, and the elevated_only_* tests). CI runs them.

Approved intent and contract text: each sentence was compared byte for byte against its approved wording.

Reviews

  • Two routine review rounds, then a final whole-delta review: approved, no blocking findings.
  • Each conformance-suite change was written blind to the implementation and reviewed separately.
  • An intent-drift audit of the governed paths found no violation. Its one partial is listed under open questions.

Conformance-suite changes (each approved before it was written)

  • Tokens are submitted with identity enroll - on standard input.
  • a_renew_transient_failures_retry_without_state_change sets __debug__.identity.min_renewal_interval_secs: 0, because it requests three renewals in one run.
  • On Windows, write_pending creates any missing directory owned by the current user, with the suite's protected DACL. Under UAC elevation a new directory is owned by Administrators, which the agent now refuses.

Deviation decided inside the brief

A pending-key recovery that starts again before any confirm succeeds now waits a backoff. The backoff grows until a confirm succeeds, capped at 10 minutes, or at backoff_max_secs when that is lower. The first recovery still starts at once.

  • Basis: contract §8.1 "retry with backoff", and the stated purpose of the 10-minute wait in crates/agent-identity/INTENT.md §Schedules.
  • Why 10 minutes: it keeps the recovery within the renewal grace of a leaf of at least 15 minutes.
  • No intent sentence states this backoff yet.

UNTESTED properties

  • devolutions-agent/src/identity/runner.rs:274: a successful confirm records last_renewal, and every new renewal starts through renewal_start.
  • devolutions-agent/src/identity/runner.rs:440: each recovery waits the backoff of recoveries. Only a successful confirm resets it; a successful renew leaves it alone.
  • crates/private-fs/src/unix.rs:201: a repair of access only reports a directory it can't open that is owned by an account the rule doesn't allow. It does not change that directory.
  • crates/agent-identity-conformance/src/windows.rs:284: a directory created concurrently, such as by the agent provisioning at start-up, is left as it is.

Open questions for a human

  1. Foreign-owned directory mid-run. crates/agent-identity/INTENT.md §Local state says a foreign-owned identity directory "stops all identity processing until it is fixed". At start-up it does. Mid-run, an owner change by an administrator makes every state change fail, but live runners and their streams keep running. Either the pending poll treats this refusal like an untrusted stored identity, or the sentence is scoped to start-up.
  2. Wording. The intent's test setting "shortens" the wait in crates/agent-identity/INTENT.md §Schedules, but "replaces" it in devolutions-agent/src/identity/INTENT.md §Configuration. The code replaces it.
  3. DVLS leaf lifetime. crates/agent-identity-conformance/README.md asks for a DVLS leaf lifetime of roughly 10–30 seconds, which contract §4 now rules out (at least 15 minutes).
  4. Short leaf at the end of a root's life. Contract §4 caps a leaf's lifetime at the issuing root's notAfter. A leaf issued close to that date can be shorter than the 10-minute wait, and then its renewal would fall beyond grace.
  5. Coverage of the default wait. No conformance case covers the default 10-minute wait between renewals. Only renewal_start unit tests and the UNTESTED wiring above do.

Non-blocking review findings left as they are

  • OwnerChange is declared once in each private-fs backend.
  • The HTTP and gRPC clients each carry their own copy of the error-code filter and the one-day offset constant. They are kept in step by tests on both sides.
  • Two closure test cases (digit_first, underscore_first) and the exact escape spelling in the loggable tests check the client's own rule, not a contract statement.
  • ForeignIdentityDirectory's message has two clauses.

Note

LLM-assisted content (no human feedback).

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>
…t-only lifetime

The certificate lifetime setting is at least 15 minutes, so the
expiry-boundary test's leaves of 10 to 30 seconds come only from the
test-only lifetime DVLS uses in its unsafe test mode.
`--leaf-lifetime-secs` therefore matches the certificate lifetime
setting, or the test-only lifetime when DVLS uses it.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The agent starts a renewal no sooner than a set wait after its previous
renewal completed, so that an authority cannot keep it renewing in a
tight loop. The new mock case shortens that wait to 15 seconds through
`min_renewal_interval_secs` and lets one requested renewal complete.
Once the agent has held one connected stream with no further
authenticated connect for 1.5 seconds, the case pushes another request,
at least 4 seconds before the wait ends.

The reference for the wait is the later of the moment before the first
request and the start of the last poll of the mock's requests that
showed no confirm, since the agent completes the renewal only after its
confirm arrives. The next renew must arrive no sooner than the wait
after that reference and within the wait plus 10 seconds after the
second request. In the mock's event order after the second request, no
stream may open or authenticate and no check-in may arrive before the
first renew, so that only the pushed request can have triggered it.
Once that renewal settles, the agent must have acknowledged the request
within the suite's usual 20-second wait; the case does not require the
acknowledgment before the wait between renewals ends, since the
contract sets no deadline for it.

`poll!` now evaluates its wait once, so that a wait computed from the
clock gives the whole duration rather than shrinking on every attempt.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The conformance suite now checks the wait between renewals for a
renewal requested on the channel, so the note on the untested wiring
names the part that stays untested: a scheduled renewal.

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

Benoît Cortier (CBenoit) commented Oct 29, 2026 •

Copy link
Copy Markdown
Member Author

Update (head 2cc351d2b)

Open questions, as answered

  1. Foreign-owned directory mid-run: to be fixed in a separate pull request stacked on this one, so that a mid-run owner change stops identity processing as at start-up; nothing changes here.
  2. "Shortens" vs "replaces": the wording stays as it is.
  3. DVLS leaf lifetime in the conformance README: fixed. The README now takes the 10–30 second leaves of the expiry-boundary test from DVLS's test-only lifetime in its unsafe test mode, and says the certificate lifetime setting stays at least 15 minutes.
  4. Short leaf at the end of a root's life: accepted as a known gap. A leaf issued close to its root's notAfter can be shorter than the 10-minute wait, so its renewal can fall beyond grace.
  5. Coverage of the wait between renewals: a new conformance case covers it with the wait shortened through the test-only setting (below).

New conformance case a_renewal_waits_between_renewals

  • Mock only. It sets __debug__.identity.min_renewal_interval_secs to 15, leaves renewal_after_secs unset, lets one requested renewal complete, and once the agent holds one stream, requests another renewal at least 4 seconds before the wait ends.
  • The next renew must come no sooner than the wait after the first renewal completed, measured from a point no later than the arrival of its confirm, and within the wait plus 10 seconds after the request.
  • In the mock's event order, no stream opens or authenticates and no check-in arrives before that renew, so only the request pushed during the wait can have triggered it.
  • The agent must acknowledge the pushed request within the suite's usual 20-second wait after the renewal settles; the contract sets no deadline for the acknowledgment, so the case does not require it before the wait ends.
  • Written without reading the implementation and reviewed in four rounds, the last by a fresh reviewer, which approved it.
  • poll! now evaluates its wait once, so that a wait computed from the clock is not shortened on every attempt.

Not checked: the default 10-minute wait itself, the wait for a scheduled renewal or after a certificate_expired refusal, and the exemption of renewal_after_secs.

Verification at this head

Windows (unelevated, fresh TEMP):

  • cargo +nightly fmt --check clean; cargo clippy -D warnings clean on the same crates as before.
  • Unit tests: agent-channel-client 140, agent-identity 227, agent-identity-conformance 60 (+1 ignored), devolutions-agent 196+6, private-fs 25.
  • testsuite private_fs: 49.
  • ci/check-oracle-markers.ps1 -Base origin/master exits 0.

Linux (WSL Ubuntu):

  • clippy clean.
  • Unit tests: agent-channel-client 140, agent-identity 210, devolutions-agent 130+6, private-fs 14.
  • testsuite private_fs: 39.
  • Conformance suite (file backend): PASS 193, FAIL 0, SKIP 0, N/A 2. Both N/A cases are Windows-only. The new case took 28 seconds.
  • The same three agent-identity-conformance unit tests as before fail only on the Windows drive mounted in WSL, which ignores chmod.

CI at the previous head passed every job; there, the conformance suite took about 8 minutes on Linux, 11 on Windows and 10.5 on macOS, within its 14.5-minute limit.

UNTESTED properties, changed

  • devolutions-agent/src/identity/runner.rs:274 now says the conformance suite checks the wait for a renewal requested on the channel; the wiring for a scheduled renewal stays untested.

Known limits of the new case

  • An unforced transport reconnect during the wait would fail the event-order check, since the new stream's Welcome re-sends the request.
  • It fails when settling the first renewal and requesting the second takes more than 11 seconds; on a loaded runner, a longer wait would widen that budget.

Note

LLM-assisted content (no human feedback).

…nt's keys

The `agent-identity` crate takes the extra trusted root, the access
grant for the current user and the renewal schedule override as plain
parameters; the agent's `__debug__.identity` section that supplies them
is defined by the agent, which names each key. The crate's intent and
documentation now describe the settings by what they do, so they hold
for any caller.

Issue: DGW-478
Security: yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CBenoit
Benoît Cortier (CBenoit) merged commit db82278 into master Oct 30, 2026
53 checks passed
@CBenoit
Benoît Cortier (CBenoit) deleted the claude/sec-agent-findings-0f32bb branch October 30, 2026 16:45
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