feat(pedm): account tracking #1319

allan2 · 2025-04-26T02:00:12Z

This commit adds tracking of user accounts and domain accounts. Changes to accounts on the system such as name change, creation, removal, and even SID change are captured.

Accounts now use an internally generated stable ID. This stable ID can be used to build policies in a robust manner.

The functionality has been tested with real account creation, deletion, and removal on Windows for both DB backends.

This commit also includes query helpers for parametrization and bulk insertion.

A basic endpoint, /accounts, is added for displaying info about existing accounts in a fashion similar to Get-LocalUser.

github-actions · 2025-04-26T02:00:27Z

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

CBenoit · 2025-04-26T14:44:52Z

crates/devolutions-pedm/src/account.rs

+///
+/// `LookupAccountNameW` must be called to enable `ConvertSidToStringSidW` to work.
+#[cfg(target_os = "windows")]
+#[allow(clippy::multiple_unsafe_ops_per_block)]


issue: Do not allow this. Put a SAFETY comment for each unsafe operation.

Addressed in 0b118c0.

CBenoit · 2025-04-26T14:45:20Z

crates/devolutions-pedm/src/account.rs

+/// `LookupAccountNameW` must be called to enable `ConvertSidToStringSidW` to work.
+#[cfg(target_os = "windows")]
+#[allow(clippy::multiple_unsafe_ops_per_block)]
+#[allow(clippy::cast_possible_truncation)]


issue: Justify why this lint is disabled.

The unsafe block was split into smaller blocks in 0b118c0.
The cast_possible_truncation exception is removed in 43b6fd2.

CBenoit · 2025-04-26T14:47:06Z

crates/devolutions-pedm/src/account.rs

+#[cfg(target_os = "windows")]
+#[allow(clippy::multiple_unsafe_ops_per_block)]
+#[allow(clippy::cast_possible_truncation)]
+pub(crate) unsafe fn list_accounts() -> Result<Vec<Account>, ListAccountsError> {


question: Why this function is marked as unsafe? What are the safety preconditions to uphold in order to call it? It seems to me that it is wrapping unsafe operations in a way that can be proven safe locally (unless there are other "ambient" preconditions?).

Addressed in 46a4fc4.

CBenoit · 2025-04-26T14:51:33Z

crates/devolutions-pedm/src/account.rs

question: This sounds similar, but slightly different from the primitives provided by the identity module of win-api-wrappers. Can you elaborate why you had to implement a different abstraction?

It was to split out the domain ID from the SID, since the domain ID portion is stored separately in the schema. I intend to use the domain ID and SID parts separately when defining policies. The domain ID will come into play for enterprises with several domains.

I saw that win-api-wrappers has lookup_account_by_name which also calls LookupAccountNameW but I needed NetUserEnum. I hope it is ok to use the windows directly as a dependency. I'd like to add more functionality directly from windows (with proper safety documentation).

CBenoit · 2025-05-20T16:03:20Z

Hi @allan2
Could rebase this PR on master please? Thank you!!

This commit adds tracking of user accounts and domain accounts. Changes to accounts on the system such as name change, creation, removal, and even SID change are captured. Accounts now use an internally generated stable ID. This stable ID can be used to build policies in a robust manner. The functionality has been tested with real account creation, deletion, and removal on Windows for both DB backends. This commit also includes query helpers for parametrization and bulk insertion. A basic endpoint, `/accounts`, is added for displaying info about existing accounts in a fashion similar to `Get-LocalUser`.

The function works as expected. The expected side of the test was wrong.

thenextman · 2025-05-21T13:33:59Z

I've had a look through these changes and I do think it's a worthwhile improvement.

That being said, I'm questioning whether we want to merge this now given that we're in the 11th hour for 2025.2 release.

If we go ahead with this, we'll need to restructure the way data is managed internally; that's probably not a huge issue since the rest of the configuration stuff is not yet ported to SQLite. However if we do see it through, it does have an impact:

We don't have time to thoroughly test this
It implies a whole bunch of refactoring on the RDM side, which I also don't see that we have time to do. Work on RDM at this point should be tweaks and bug fixes, not reworking the UX.

What's the side effect of not having these changes? Well, we won't track the changes that @allan2 wrote in the PR notes but honestly I'd consider that to be a bit of an edge case

User SID changes are not common. The main reason that impact us would be that the user gets moved from one Active Directory to another (noting that in this case the SID is not really "changed": a new user is created in the new AD, the history is copied over, and the old is deleted)
User name changes are still uncommon, but perhaps more of a worry. What happens if a user name changes right now? I didn't try it, but it's likely that the profile assignment will be lost (but reassigning to the "new" user is quite easy) and the log interface will be weird (because logs will be available for both the old and the new username). I don't see either of those as a particular problem, given the disruption that occurs in general when you rename a Windows user.

It seems trivial to me to go with what we have for now, and after merging this it should not be too troublesome to migrate the data in the user table to account; that's just an internal representation and can be done easily; the real work is in adjusting the APIs and ensuring we get a consistent experience in RDM. I don't think there's time to do that now.

A middle ground would be to merge this but not integrate it at this point. As long as we are assured that the account sync is not introducing issues in general in the service,.

thenextman · 2025-05-21T13:36:19Z

And furthermore: if we get a case where the customer is having issues due to a renamed user or an altered SID; it's relatively easy to just recreate the configurations on the RDM side. There are very few options. If it so happened that we had a customer deploy this, with many dozens of users on the machine and lots of configuration (very unlikely at this point) and they were affected by those circumstances, it could be fixed directly in the DB. I think coding around the problem is worthwhile, but not something we have time to put our attention on this week.

CBenoit reviewed Apr 26, 2025

View reviewed changes

allan2 added 6 commits May 20, 2025 12:56

Fix query args test typo

b9af8cb

The function works as expected. The expected side of the test was wrong.

Split unsafe blocks and add safety comments

08e409a

Use u32::try_from

c7126f0

Unmark list_accounts as unsafe

71f20c4

Rebase master

d58bc90

allan2 force-pushed the pedm-account branch from acb4711 to d58bc90 Compare May 20, 2025 17:05

Remove mistakenly added build.rs

409200f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(pedm): account tracking #1319

feat(pedm): account tracking #1319

Uh oh!

allan2 commented Apr 26, 2025

Uh oh!

github-actions bot commented Apr 26, 2025

Uh oh!

CBenoit Apr 26, 2025

Uh oh!

allan2 May 13, 2025

Uh oh!

CBenoit Apr 26, 2025

Uh oh!

allan2 May 13, 2025

Uh oh!

CBenoit Apr 26, 2025 •

edited

Loading

Uh oh!

allan2 May 13, 2025

Uh oh!

CBenoit Apr 26, 2025

Uh oh!

allan2 May 13, 2025

Uh oh!

CBenoit commented May 20, 2025

Uh oh!

thenextman commented May 21, 2025

Uh oh!

thenextman commented May 21, 2025

Uh oh!

Uh oh!

feat(pedm): account tracking #1319

Are you sure you want to change the base?

feat(pedm): account tracking #1319

Uh oh!

Conversation

allan2 commented Apr 26, 2025

Uh oh!

github-actions bot commented Apr 26, 2025

Let maintainers know that an action is required on their side

Uh oh!

CBenoit Apr 26, 2025

Choose a reason for hiding this comment

Uh oh!

allan2 May 13, 2025

Choose a reason for hiding this comment

Uh oh!

CBenoit Apr 26, 2025

Choose a reason for hiding this comment

Uh oh!

allan2 May 13, 2025

Choose a reason for hiding this comment

Uh oh!

CBenoit Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allan2 May 13, 2025

Choose a reason for hiding this comment

Uh oh!

CBenoit Apr 26, 2025

Choose a reason for hiding this comment

Uh oh!

allan2 May 13, 2025

Choose a reason for hiding this comment

Uh oh!

CBenoit commented May 20, 2025

Uh oh!

thenextman commented May 21, 2025

Uh oh!

thenextman commented May 21, 2025

Uh oh!

Uh oh!

CBenoit Apr 26, 2025 •

edited

Loading