feat: add elevated execution support to exec messages - #102
feat: add elevated execution support to exec messages#102Richard Markiewicz (thenextman) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The Rust all-capabilities mask can advertise reserved token elevation, and the .NET batch client cannot request the new NO_EXIT behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds NOW-PROTO 1.7 elevated-execution intent, capability negotiation, and batch NO_EXIT support across Rust and .NET.
Changes:
- Adds elevation flags and capability bits.
- Preserves Rust Run flags during decoding.
- Adds cross-language wire-format tests and specification updates.
File summaries
| File | Description |
|---|---|
protocols/rust/now-proto-testsuite/tests/proto/exec.rs |
Tests elevated exec flags. |
protocols/rust/now-proto-testsuite/tests/proto/channel.rs |
Updates version snapshots. |
protocols/rust/now-proto-pdu/src/exec/win_ps.rs |
Adds WinPS elevation. |
protocols/rust/now-proto-pdu/src/exec/shell.rs |
Adds shell elevation. |
protocols/rust/now-proto-pdu/src/exec/run.rs |
Adds elevation and preserves decoded flags. |
protocols/rust/now-proto-pdu/src/exec/pwsh.rs |
Adds pwsh elevation. |
protocols/rust/now-proto-pdu/src/exec/process.rs |
Adds process elevation. |
protocols/rust/now-proto-pdu/src/exec/batch.rs |
Adds elevation and NO_EXIT. |
protocols/rust/now-proto-pdu/src/channel/capset.rs |
Adds capabilities and version 1.7. |
protocols/dotnet/Devolutions.NowProto/src/NowProtoVersion.cs |
Updates protocol version. |
protocols/dotnet/Devolutions.NowProto/src/Messages/NowMsgExecWinPs.cs |
Adds WinPS elevation. |
protocols/dotnet/Devolutions.NowProto/src/Messages/NowMsgExecShell.cs |
Adds shell elevation. |
protocols/dotnet/Devolutions.NowProto/src/Messages/NowMsgExecRun.cs |
Adds Run elevation. |
protocols/dotnet/Devolutions.NowProto/src/Messages/NowMsgExecPwsh.cs |
Adds pwsh elevation. |
protocols/dotnet/Devolutions.NowProto/src/Messages/NowMsgExecProcess.cs |
Adds process elevation. |
protocols/dotnet/Devolutions.NowProto/src/Messages/NowMsgExecBatch.cs |
Adds elevation and NO_EXIT. |
protocols/dotnet/Devolutions.NowProto/src/Capabilities/NowCapabilityExec.cs |
Defines elevation capabilities. |
protocols/dotnet/Devolutions.NowProto.Tests/src/NowExecStyles.cs |
Tests exec encodings. |
protocols/dotnet/Devolutions.NowProto.Tests/src/MsgChannel.cs |
Updates version snapshots. |
protocols/dotnet/Devolutions.NowClient/src/ExecWinPsParams.cs |
Exposes WinPS elevation. |
protocols/dotnet/Devolutions.NowClient/src/ExecShellParams.cs |
Exposes shell elevation. |
protocols/dotnet/Devolutions.NowClient/src/ExecRunParams.cs |
Exposes Run elevation. |
protocols/dotnet/Devolutions.NowClient/src/ExecPwshParams.cs |
Exposes pwsh elevation. |
protocols/dotnet/Devolutions.NowClient/src/ExecProcessParams.cs |
Exposes process elevation. |
protocols/dotnet/Devolutions.NowClient/src/ExecBatchParams.cs |
Exposes batch elevation. |
protocols/docs/NOW-spec.md |
Documents NOW-PROTO 1.7 semantics. |
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9dd8b4d to
0c07ab1
Compare
`NowExecRunMsg::decode_from_body` discarded the header flags and rebuilt them from payload content, making it the only exec message that did not preserve flags on decode. Every other exec message seeds its flags from `from_bits_retain(header.flags)`. Any flag on NOW_EXEC_RUN_MSG other than DIRECTORY_SET was therefore silently dropped by the decoder, so a server could not observe an option the client had set. DIRECTORY_SET is still derived from the payload rather than trusted from the header, because peers older than v1.1 omit the directory field entirely. No test accompanies this on its own: NOW_EXEC_RUN_MSG has no second flag to assert against yet. Coverage arrives with the first one added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0c07ab1 to
6cda077
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The .NET client can send elevation flags without validating negotiated elevation support, allowing silent downgrade against older servers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 7
- Review effort level: Balanced
6cda077 to
97311d3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Batch NO_EXIT silently degrades against pre-1.7 servers, and its individual wire-bit allocations are not independently tested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 3
- Review effort level: Balanced
| public ExecBatchParams NoExit(bool enable = true) | ||
| { | ||
| _noExit = enable; | ||
| return this; |
There was a problem hiding this comment.
The concern is real but I'd rather not fix it this way, because the remedy would make NoExit the only version-gated exec flag in the library while its exact siblings stay ungated.
State of the code today:
NowClientversion-gates exactly one thing: the whole RDM extension (MIN_RDM_ENABLED_VERSION = 1.3, checked inEnsureRdmCapabilities).- It version-gates zero exec flags.
DETACHEDarrived in 1.4 with the identical property you describe — send it to a 1.3 server and the session is tracked anyway, silently contradicting the caller.RAW_ENCODINGandUNICODE_CONSOLEarrived in 1.6; the latter got a capability bit,NOW_CAP_EXEC_UNICODE_CONSOLE, rather than a version check.
So the two coherent options are "the library gates no exec flags and callers gate" (status quo — RDM gates fire-and-forget on Version >= 1.4 and Unicode console on the capset bit) or "the library gates all of them, consistently". Gating just the newest flag is the one option that leaves the codebase harder to
reason about.
There's also a severity gap worth naming. Elevation silently not happening is a trust and correctness failure: the operator believes a script ran as administrator when it did not, and nothing in the UI or the logs says otherwise - that is the exact bug this PR exists to close, which is why it gets a hard refusal client-side. NoExit silently not applying means a console window closes instead of staying open: immediately visible to the person watching the remote desktop, no incorrect belief about privileges, no correctness impact. The enforcement asymmetry is proportionate to the consequence asymmetry.
Elevation also had a mechanism that a version can never express (shell vs token elevation differ in whether stdio survives), which is why it needed capability negotiation rather than a version compare. NoExit has no such dimension.
Concretely, my preference: leave the library as is, and have RDM gate the Keep open checkbox the same way it already gates Fire and forget. That is in the RDM-side plan for this work. If you'd rather have library-level enforcement, I think it deserves its own PR that covers DETACHED and the encoding flags too, so the rule is uniform.
f6523d6 to
b54b39a
Compare
Marc-André Moreau (mamoreau-devolutions)
left a comment
Marc-André Moreau (mamoreau-devolutions)
left a comment
There was a problem hiding this comment.
quick checkup with some questions
| /// | ||
| /// NOW-PROTO: NOW_CAP_EXEC_ELEVATE_TOKEN | ||
| /// </summary> | ||
| ElevateToken = 0x0100, |
There was a problem hiding this comment.
what's the difference between NOW_CAP_EXEC_ELEVATE_SHELL and NOW_CAP_EXEC_ELEVATE_TOKEN? what are the mechanics for dealing with elevated primary tokens over the wire? I guess I'll find out later in this PR
There was a problem hiding this comment.
ELEVATE_SHELL is what we'll support today. It's a capability downgrade (no I/O redirection, for instance) but can be done quite easily.
ELEVATE_TOKEN is just a reservation, no-one advertise or implements it as a capability now. But in the future, it allow us to offer an alternative approach where elevation is handled inside the service and we support the current I/O redirection (service will elevate without UAC prompt, I/O redirection by IPC into the user session / devolutions-session).
By adding both now, it just becomes a capability flip in the future.
| /// | ||
| /// NOW-PROTO: NOW_EXEC_FLAG_RUN_ELEVATED | ||
| /// </summary> | ||
| Elevated = 0x0002, |
There was a problem hiding this comment.
do we have a proper mechanism for reporting errors related to trying to launch commands elevated, and it failed in some way? things like timeout from the UAC prompt, or the user denying the UAC prompt, etc.
There was a problem hiding this comment.
Partly. Existing code routes ShellExeute to NowStatusError::new_winapi(code) so a declined prompt for example comes back as ERROR_CANCELLED (1223).
We could improve the vocabulary because dealing with a Win32 error code client side is not very nice.
That said, there's no way to distinguish a timed out UAC prompt to a declined one. They're both "cancelled".
Updates NOW-PROTO to v1.7, adding the ability for a client to request that an exec session run with elevated privileges. - Add `NOW_EXEC_FLAG_*_ELEVATED` to the run, process, shell, batch, winps and pwsh exec messages. The flag expresses intent only and never names a mechanism, so the server remains free to change how it elevates. It raises the privileges of the identity that would otherwise run the command and carries no credentials; running a command as a different user is a separate concern for a separate field. - Add `NOW_CAP_EXEC_ELEVATE_SHELL`, advertised by servers that elevate through the platform shell (consent prompt, no stdio redirection). It is the only elevation capability defined: a mechanism with different observable properties is expected to arrive as an additional capability rather than by redefining this one, so a client always learns what to expect before it sends. - Refuse an elevated request in `NowClient` unless the negotiated capset advertises elevation. A server older than 1.7 does not know the flag and would execute the command without elevation, and the caller has no way to detect that, so the request is rejected locally rather than downgraded silently. - Specify the mechanism-dependent semantics in a new "Elevated Execution" section: how elevation is requested, what it does and does not mean, that a server must fail rather than silently execute unelevated, that capability negotiation is the only reliable signal because older servers ignore the flag, and what the shell mechanism implies for stdio, result reporting and abort. - Add `NOW_EXEC_FLAG_BATCH_NO_EXIT` (`cmd /K` rather than `/C`), so an elevated batch session, which has no stdio, can still leave its console open for the user to read. Must be ignored when the session redirects stdio. Exposed on `ExecBatchParams` alongside the existing winps and pwsh `NoExit` options. The round-trip tests added here are also the first coverage of the RUN decoder fix in the preceding commit: ELEVATED is the first NOW_EXEC_RUN_MSG flag that would have been dropped by it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b54b39a to
7c60d1e
Compare
Summary
Updates NOW-PROTO to v1.7, adding the ability for a client to request that an exec session run with elevated privileges.
RDM has had a "Run as administrator" checkbox in Execute script via Devolutions Agent since the legacy RDM Jump days, where it worked (the receiving RDM ran
ShellExecutewith therunasverb). The jump transport was removed inRDMW-21914, and the NOW exec path has no way to carry the intent so the checkbox has silently done nothing since.
This PR adds the protocol half.
What this adds
NOW_EXEC_FLAG_*_ELEVATEDon the run, process, shell, batch, winps and pwsh exec messages.NOW_CAP_EXEC_ELEVATE_SHELL(0x0080), advertised by servers that elevate through the platform shell: may prompt the interactive user for consent, and cannot redirect stdio.NOW_CAP_EXEC_ELEVATE_TOKEN(0x0100), reserved and unimplemented - elevation by assigning an elevated primary token, which preserves IO redirection and raises no prompt. Deliberately excluded fromNowCapabilityExec.Allso no peer can advertise it yet.NOW_EXEC_FLAG_BATCH_NO_EXIT(0x0010) —cmd /Krather than/C. Under shell elevation there is no stdio, so a console left open is the only way the user sees output; winps/pwsh already haveNO_EXIT, batch did not. MUST be ignored when the session redirects stdio, where the hidden interpreter would never exit.Design: intent, not mechanism
The main thing to review. Two rules, both aimed at letting the token mechanism land later without a breaking change:
ELEVATED, neverRUNAS. A client cannot select a mechanism, so a server is free to change how it elevates without any client change.execCapset, the same wayUNICODE_CONSOLEandIO_REDIRECTIONalready are. A client reads the capability and knows before sending what elevation will cost it.The spec section pins down what each mechanism means for stdio, result reporting and abort, so tightening any of it later is a documented capability change rather than a silent behaviour change. It also requires that a server fail rather than execute unelevated after being asked to elevate. Silently downgrading is the exact defect this work exists to fix.
Bit allocation
Each exec message has its own flag namespace; values were picked to avoid every bit already in use.
NOW_EXEC_RUN_MSGNOW_EXEC_FLAG_RUN_ELEVATED0x0002NOW_EXEC_PROCESS_MSGNOW_EXEC_FLAG_PROCESS_ELEVATED0x0008NOW_EXEC_SHELL_MSGNOW_EXEC_FLAG_SHELL_ELEVATED0x0008NOW_EXEC_BATCH_MSGNOW_EXEC_FLAG_BATCH_ELEVATED0x0008NOW_EXEC_BATCH_MSGNOW_EXEC_FLAG_BATCH_NO_EXIT0x0010NOW_EXEC_WINPS_MSG/NOW_EXEC_PWSH_MSGNOW_EXEC_FLAG_PS_ELEVATED0x0800Included bug fix
NowExecRunMsg::decode_from_bodydiscarded the header flags and rebuilt them from payload content, making it the only exec message that did not preserve flags on decode. Any flag added toNOW_EXEC_RUN_MSG- includingELEVATED- was silently dropped by the decoder, so the agent would never have seen the request. Now seeded fromheader.flags;DIRECTORY_SETis still derived from the payload because peers older than v1.1 omit the directory field entirely.Caught by the new round-trip test. Happy to split this into its own
fix:commit if you want it to appear as a separate changelog entry — it's a latent bug in released 0.4.4, independent of this feature.Testing
ELEVATED(andNO_EXITfor batch) survives encode → decode for every exec style.rust fmt,dotnet fmt,check typos,rust tests(60),rust lints,dotnet tests(59),dotnet build,check locks— all green.Notes for the reviewer
NOW-spec.pdfnot regenerated. The PDF is not tracked in git.supports_exec_elevation()version helper, deviating from the 1.6 precedent which addedsupports_exec_unicode_console(). Elevation is meant to be gated on the capability bit, not the version, and a version helper would invite the wrong pattern.Follow-up work
This PR is protocol-only; nothing changes behaviour yet.
ELEVATEDinprocess_exec_run(verb selection, the smallest increment), advertiseELEVATE_SHELL, and map consent-declined (ERROR_CANCELLED) to a distinct status.win_api_wrappers::process::shell_executeso elevated sessions still report a real exit code.