Skip to content

fix(agent): private key permissions on Windows - #1931

Open
Kyle Bouchard (ptrstr) wants to merge 1 commit into
masterfrom
DGW-442-fix-cert-permissions
Open

fix(agent): private key permissions on Windows#1931
Kyle Bouchard (ptrstr) wants to merge 1 commit into
masterfrom
DGW-442-fix-cert-permissions

Conversation

@ptrstr

Copy link
Copy Markdown

Private keys for the agent tunnel are now created with an explicit access control list limited to SYSTEM and Administrators, instead of inheriting the permissions of the Devolutions data directory. Previously these files were readable by any local user on the machine: the tunnel CA and server keys on a Gateway host, and the tunnel client key on an enrolled Agent.

Existing keys keep their current permissions. I'm not sure how widely deployed/used this feature was, so let me know if a migration is required.

Copilot AI balanced review requested due to automatic review settings August 12, 2026 19:40
@github-actions

Copy link
Copy Markdown

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f07c58984c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment on lines +295 to +296
#[cfg(windows)]
restrict_to_system_and_admins(&client_key_path)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Apply the DACL when creating the key file

On Windows MSI installations, the Agent data directory grants built-in users read access to inherited children, but this call runs only after std::fs::write has created, populated, and closed the key file. A local user monitoring the certs directory can therefore open the temporarily inherited-readable file before this call; changing its DACL afterward does not revoke that handle, allowing the private key to be copied. Create the file with the restrictive security descriptor atomically, or write it in a pre-secured location before moving it into place.

Useful? React with 👍 / 👎.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Windows ACL restrictions for newly enrolled Agent tunnel client keys.

Changes:

  • Builds a protected DACL granting access only to SYSTEM and Administrators.
  • Applies the DACL during enrollment persistence.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +295 to +296
#[cfg(windows)]
restrict_to_system_and_admins(&client_key_path)?;
Comment on lines +295 to +296
#[cfg(windows)]
restrict_to_system_and_admins(&client_key_path)?;
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.

2 participants