New Module: Manage Windows local group membership (win_group_member)

[andrewsaraceni]

SUMMARY

Currently, no module exists for managing membership of local groups on Windows, as discussed in #20428. This module allows for the addition and removal of members of local groups on Windows, including: local, service and domain users, and domain groups. Syntax for member specification allows for/parses the following conventions:

• username
• .\username
• SERVERNAME\username
• NT AUTHORITY\username
• DOMAIN\username
• username@DOMAIN

Integration tests are included as well.

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

lib/ansible/modules/windows/win_group_member.ps1

ANSIBLE VERSION

ansible 2.4.0

ADDITIONAL INFORMATION

- name: Add a local and domain user to a local group
  win_group_member:
    name: Remote Desktop Users
    members:
      - DOMAIN\TestUser
      - NewLocalAdmin
    state: present

- name: Remove a domain group and service user from a local group
  win_group_member:
    name: Backup Operators
    members:
      - DOMAIN\TestGroup
      - NT AUTHORITY\SYSTEM
    state: absent

[ansibot]

@SamLiu79 @timothyvandenbrande @ar7z1 @blakfeld @brianlloyd @chrishoffman @if-meaton @joshludwig @petemounce @schwartzmx @smadam813

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

[jborean93]

This looks really good to me, fantastic work. The only thing I would recommend is to create a test group(s) in the integration tests instead of changing an inbuilt one. This way the tests become non-destructive if you remove the temp groups in the always part of your block.

I'll leave it to @nitzmahone @dagwieers @jhawkesworth @trondhindenes to see if they have any changes they wish to add.

[andrewsaraceni]

Thanks @jborean93! I'll make that change to the tests later today.

[jborean93]

Hey @andrewsaraceni I've just had another look at this and have a concern around the SID lookup function you have.

You might want to check this with a domain joined computer and validate the SID it returns when the local user also exists in the domain. I believe if you specify only the username argument to NTAccount and that username exists as both a local and domain user it will return the domain user. In this case it will return the SID for the domain user when in fact you want the local user. The trouble is that for local groups you can't set the hostname argument or it would fail.

I went a similar route for determining the SID for a new module https://github.com/jborean93/ansible/blob/e3c728e3229b117501a1109dbeb050f3b680ee83/lib/ansible/modules/windows/win_user_right.ps1#L51-L113. I unfortunately don't have access to my test domain environment so can't 100% verify it.

[andrewsaraceni]

Hey @jborean93, thanks for looking into this. I just tested this on a domain joined machine, and my testing actually shows the opposite. It looks like when a user exists only in the domain, and NTAccount/SID translation is performed with only username, the domain user is returned. However, upon the creation/existence of a local account with same username as the domain user sAMAccountName, it instead returns the local user.

Here's a test script I used:

function Test-SidResolution {
    param($UserToTest)
    $sid_check_args = @($UserToTest)
    $user_object = New-Object -TypeName System.Security.Principal.NTAccount -ArgumentList $sid_check_args
    return $user_object.Translate([System.Security.Principal.SecurityIdentifier]).Value
}

# test user - only exists in domain initially...
$user = "testuser"
$val1 = Test-SidResolution -UserToTest $user

# create local user with same username (e.g. same as domain sAMAccountName)
$objOu = [ADSI]”WinNT://$env:COMPUTERNAME“
$objUser = $objOU.Create(“User“, $user)
$objUser.SetInfo()

# test user again after creating locally
$val2 = Test-SidResolution -UserToTest $user

"`n$val1`n" | Write-Output
"`n$val2`n" | Write-Output

S-1-5-21-<domain>-17761


S-1-5-21-<localComputer>-1008

With that being said, I could see a case where a false positive occurs with the current code. e.g. A lookup is done for (an intended) local user, and only using username it returns the user exists, however they exist only in the domain and not locally. I think removing the conditional argument building in 89-96, and simply instantiating NTAccount with domain and username every time (including when domain is $env:COMPUTERNAME) would solve this.

Windows doesn't allow nesting of local groups - and therefore, this module doesn't - so excluding that case, doing SID resolution for users (local and domain), groups (domain) and service accounts should be possible in every case with domain and username.

Let me know what you think. If that makes sense to you, I could change this.

[jborean93]

Thanks for the info, I'll have to retest my function and see what happens there but I have a feeling it is because I was running it on a DC which would change how things work but will verify later.

The only trouble with removing lines 89-96 is for special accounts like SYSTEM when someone doesn't specify the prefix NT AUTHORITY\. Because of this reason I have a check in my code when the domain isn't set to first search if the local user exists. If it does then initialise NTAccount with domain being $env:COMPUTERNAME. If it doesn't then it is a special account like SYSTEM and initialise NTAccount without the domain parameter.

Agreed about the local group side, didn't think of that fact.

[andrewsaraceni]

Yeah, I haven't tested this on a DC, so that's a good point.

I had pretty much figured service accounts would be added (for this module) with the NT AUTHORITY\ prefix - that's what I wrote as a requirement in the documentation for the description for members. If that isn't sufficient, still removing 89-96 and doing a more extensive check around 53-76 should catch that case, like another elseif before the else on 72:

elseif ($GroupMember -in @("SYSTEM", "NETWORK SERVICE", "LOCAL SERVICE")) {
    # Service account w/o domain specified, e.g. SYSTEM
    $parsed_member.domain = "NT AUTHORITY"
    $parsed_member.username = $GroupMember
}

Otherwise, it'd probably be a heavier refactoring to do something like you did for win_user_right, sans some of the logic for handling/checking local groups. Do you have a particular preference?

[jborean93]

I can confirm running on a DC means it favours the DC principals, makes sense why as it is a DC. I'm not a fan having a check in place that checks well know usernames as Microsoft tends to add more NT AUTHORITY accounts in new Windows releases and I wouldn't want to maintain a static list. I'll add it to the agenda for the Windows working group to discuss but I think handling people putting in SYSTEM is something we should have as other modules currently do it.

For some background we have quite a few modules that convert usernames to SID's and when the modular powershell utils is implemented we will be looking for a way to standardise the process which means handling local groups would be required. I am hoping that this module would be either a drop dead replacement of close to it when that happens.

[andrewsaraceni]

Got it, that makes sense. I may be able to join the working group discussion in IRC for a bit. In any case, it's good to see this getting discussed.

Depending on the outcome of that discussion, but also generally, I'll look into improving this for handling service accounts more liberally, and tackling the false positive case I mentioned before.

[andrewsaraceni]

@jborean93 Something else I just realized...

For the purpose of this module, and the way the ADSI class/accelerator seems to work in general (based on my testing), service accounts have to be added and removed from groups with an ADsPath including the NT AUTHORITY prefix, e.g. WinNT://NT AUTHORITY/SYSTEM.

If someone wanted to pass only say SYSTEM without the NT AUTHORITY\ prefix to the module, though we'd be able to do a SID translation, we'd still have to flag it as a service account and add NT AUTHORITY as the domain/hostname identifier before we work with it. This again comes back to ideas like the static array of account names, or ideally a better option if it exists (e.g. WMI lookup, a native .NET enum check).

Whether of not that logic has to live within the Test-GroupMember function, or a PowerShell module utils version of it is definitely up for debate, but just something to consider.

[jborean93]

@andrewsaraceni we discussed this in the meeting and the consensus was it would be good to handle specifying service accounts like SYSTEM without the NT AUTHORITY prefix. I understand that this won't work with your process in the current setup and think we should just merge as is and refactor it later to support this behaviour.

[jborean93]

@nitzmahone I'm pretty happy with how this is so far are you able to have a look. There are a few thing that would be good to change in the future but that shouldn't hold up getting this module in.

[dagwieers]

+label new_module

[nitzmahone]

Yep, definitely a few things we can tighten up and refactor over time, but we definitely want to get this one in. Thanks!