feat(pedm): account tracking #1319
Conversation
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`.
Let maintainers know that an action is required on their side
|
The function works as expected. The expected side of the test was wrong.
| /// | ||
| /// `LookupAccountNameW` must be called to enable `ConvertSidToStringSidW` to work. | ||
| #[cfg(target_os = "windows")] | ||
| #[allow(clippy::multiple_unsafe_ops_per_block)] |
There was a problem hiding this comment.
issue: Do not allow this. Put a SAFETY comment for each unsafe operation.
There was a problem hiding this comment.
Addressed in 0b118c0.
| /// `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)] |
There was a problem hiding this comment.
issue: Justify why this lint is disabled.
| #[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> { |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
Addressed in 46a4fc4.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
This fixes some random clippy warnings in master.
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 toGet-LocalUser.