ADReadOnlyDomainControllerAccount: New resource #713

Borgquite · 2024-07-29T12:55:29Z

Pull Request (PR) description

As discussed in #711 - a new resource to allow pre-staging of read-only domain controller accounts

This Pull Request (PR) fixes the following issues

Task list

Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof and comment-based
help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

Borgquite · 2024-07-29T12:55:54Z

Just doing an initial pull to see if this works before I update the tests - leave this with me for now :)

…ODC account)

johlju · 2024-07-29T14:12:39Z

For the build to run, update azure-pipelines.yml according to this: gaelcolas/Sampler#477 (comment)

Borgquite · 2024-07-29T14:20:11Z

@johlju Love it when you pop out for lunch & the solution's waiting when you return! Ta! :)

codecov · 2024-07-29T14:30:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98%. Comparing base (bdde66f) to head (08e2163).
Report is 4 commits behind head on main.

Additional details and impacted files

@@         Coverage Diff          @@
##           main   #713    +/-   ##
====================================
  Coverage    98%    98%            
====================================
  Files        25     26     +1     
  Lines      3475   3720   +245     
====================================
+ Hits       3406   3651   +245     
  Misses       69     69

Files	Coverage Δ
...FT_ADDomainController/MSFT_ADDomainController.psm1	`100% <100%> (ø)`
...ccount/MSFT_ADReadOnlyDomainControllerAccount.psm1	`100% <100%> (ø)`
...DirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1	`95% <100%> (+<1%)`	⬆️

…urce

…iteName

…ontroller if this computer is the target

Borgquite · 2024-07-30T13:22:18Z

@johlju This PR is ready for review whenever you are :)

johlju

Awesome work, just one comment to discuss.

Reviewed 4 of 7 files at r1, 1 of 2 files at r3, 8 of 9 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Borgquite)

source/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 line 1354 at r5 (raw file):

        $domainControllerObject = Get-ADDomainController @getADDomainControllerParameters

        if (-not $domainControllerObject -and $env:COMPUTERNAME -eq $ComputerName -and (Test-IsDomainController) -eq $true)

Not sure about this change, this prevents the command to be called from another computer - feels like a breaking change. Why is it needed? 🤔

Borgquite

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

source/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 line 1354 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Not sure about this change, this prevents the command to be called from another computer - feels like a breaking change. Why is it needed? 🤔

Sure.

TL;DR Get-DomainControllerObject is currently broken when $ComputerName is not $env:COMPUTERNAME, and this fixes it.

--

Get-DomainControllerObject is used by ADDomainController and now ADReadOnlyDomainControllerAccount to find the domain controller object for $ComputerName. Until this change, it's never been used for anything other than $env:COMPUTERNAME (local node) inside ADDomainController - but I'm reusing it in ADReadOnlyDomainControllerAccount.

However, at present, if $ComputerName is set to another name, to check if a computer other than the local computer has a domain controller account, it always fails. This line basically says 'if there is no domain controller account object named $ComputerName, and the local computer is a domain controller, throw an error' (Test-IsDomainController returns true if the local computer is a domain controller). This doesn't make any sense at all if $ComputerName is not the local computer - so Get-DomainControllerObject always fails if $env:COMPUTERNAME != $ComputerName. The fix here is to bypass running Test-IsDomainController unless we’re checking the local computer’s name - which allows the function to be used in ADReadOnlyDomainController as intended.

Borgquite · 2024-08-01T10:54:21Z

@johlju Let me know if I haven't explained the above very well!

Borgquite · 2024-08-08T14:21:47Z

@johlju Ping - when you are able!

Borgquite · 2024-08-09T11:36:52Z

I've updated Get-DomainControllerObject to make the intended fix clearer

PlagueHO

Hi @Borgquite - great job! I've finished review and just a few minor things.

One thing I always ask: Is it possible to create integration tests for this resource as well? Even if they can't be executed in the Azure DevOps pipeline, being able to run them locally on a node in a test AD would be useful. I haven't kept up with this module, so it's possible that the module doesn't generally keep up this practice - so defer to the usual maintainers 😀

Reviewed 4 of 7 files at r1, 1 of 2 files at r3, 6 of 9 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @Borgquite and @johlju)

CHANGELOG.md line 23 at r6 (raw file):

- Update build process to pin GitVersion to 5.* to resolve errors
  (https://github.com/gaelcolas/Sampler/issues/477).

Can you also mention the fix to Get-DomainControllerObject?

source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.schema.mof line 20 at r6 (raw file):

    [Write, Description("Specifies one or more Flexible Single Master Operation (FSMO) roles to move to this domain controller. The current owner must be online and responding for the move to be allowed."), ValueMap{"DomainNamingMaster", "SchemaMaster", "InfrastructureMaster", "PDCEmulator", "RIDMaster"}, Values{"DomainNamingMaster", "SchemaMaster", "InfrastructureMaster", "PDCEmulator", "RIDMaster"}] String FlexibleSingleMasterOperationRole[];
    [Write, Description("Specifies if the DNS Server service should be installed and configured on the Domain Controller. If this is not set the default value of the parameter `InstallDns` of the cmdlet Install-ADDSDomainController is used. This parameter is only used during the provisioning of a domain controller. The parameter cannot be used to install or uninstall the DNS server on an already provisioned domain controller.")] Boolean InstallDns;
    [Write, Description("Indicates that the cmdlet attaches a server to an existing Read-Only Domain Controller (RODC) account. If specified, a member of the Domain Admins group or a delegated user can run this cmdlet.")] Boolean UseExistingAccount;

This seems to refer to a cmdlet. Presumably this should refer to "apply the DSC resource"?

Code quote:

If specified, a member of the Domain Admins group or a delegated user can run this cmdlet.

source/DSCResources/MSFT_ADDomainController/en-US/about_ADDomainController.help.txt line 106 at r6 (raw file):

.PARAMETER UseExistingAccount
    Write - Boolean
    Indicates that the cmdlet attaches a server to an existing Read-Only Domain Controller (RODC) account. If specified, a member of the Domain Admins group or a delegated user can run this cmdlet.

See previous comment about: "This seems to refer to a cmdlet. Presumably this should refer to "apply the DSC resource"?"

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 99 at r6 (raw file):

        # Retrieve any user or group that is a delegated administrator via the ManagedBy attribute
        $delegateAdministratorAccountName = $null
        $domainControllerComputerObject = $domainControllerObject.ComputerObjectDN | Get-ADComputer -Properties ManagedBy -Credential $Credential

nit: Long line - can this be split at the pipe (I know other resources in this module don't adhere to this - so only a nit)

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 102 at r6 (raw file):

        if ($domainControllerComputerObject.ManagedBy)
        {
            $domainControllerManagedByObject = $domainControllerComputerObject.ManagedBy | Get-ADObject -Properties objectSid -Credential $Credential

nit: Long line - can this be split at the pipe (I know other resources in this module don't adhere to this - so only a nit)

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 107 at r6 (raw file):

        }

        $allowedPasswordReplicationAccountName = (

nit: Are ( ) needed here?

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 110 at r6 (raw file):

            Get-ADDomainControllerPasswordReplicationPolicy -Allowed -Identity $domainControllerObject |
            ForEach-Object -MemberName sAMAccountName)
        $deniedPasswordReplicationAccountName = (

nit: Are ( ) needed here?

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 122 at r6 (raw file):

            DomainName                          = $domainControllerObject.Domain
            Ensure                              = $true
            InstallDns                          = $InstallDns

Is this value stored with the RODC account? If not, how does it get used when prestaging an RODC account?

I can see that Add-ADDSReadOnlyDomainControllerAccount does support the -InstallDns parameter, but I can't actually figure out how it would actually work - unless the flag was stored with the AD object.

The reason I'm asking is if the state is not stored and able to be retrieved, could we eliminate it entirely - as we're not installing an RODC (and the DNS service), we're just creating the RODC account object.

Sorry if this doesn't make sense (it's been a while since I've dealt with RODC's 😁)

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 151 at r6 (raw file):

<#
    .SYNOPSIS
        Installs, or change properties on, a read only domain controller account.

nit: Installs -> Creates?

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 288 at r6 (raw file):

        }

        if ($PSBoundParameters.ContainsKey('InstallDns'))

See previous query regarding InstallDns parameter.

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 347 at r6 (raw file):

            $delegateAdministratorAccountSecurityIdentifier = Resolve-SecurityIdentifier -SamAccountName $DelegatedAdministratorAccountName

            Set-ADComputer -Identity $domainControllerObject.ComputerObjectDN -ManagedBy $delegateAdministratorAccountSecurityIdentifier -Credential $Credential

nit: long line - can split or use splat?

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 398 at r6 (raw file):

        }

        if ($PSBoundParameters.ContainsKey('DenyPasswordReplicationAccountName'))

This block seems to have a lot of similar code to the previous block. Any chance we could split it out into a function and reduce the duplication?

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 477 at r6 (raw file):

        Provides a list of the users, computers, and groups to add to the password replication denied list.

    .PARAMETER InstallDns

See previous query regarding InstallDns

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/MSFT_ADReadOnlyDomainControllerAccount.psm1 line 609 at r6 (raw file):

    }

    if ($PSBoundParameters.ContainsKey('DenyPasswordReplicationAccountName') -and

nit: some code duplication in this block, but probably not enough to warrant splitting into a function. Will leave decision to you.

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/README.md line 8 at r6 (raw file):

DelegatedAdministratorAccountName rather than requiring Domain Admins permissions.

>The resource does not support removing pre-created Read Only Domain Controller accounts.

nit: Add space after >

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/en-US/MSFT_ADReadOnlyDomainControllerAccount.strings.psd1 line 3 at r6 (raw file):

ConvertFrom-StringData @'
    ResolveDomainName                           = Resolving the domain name '{0}'. (ADRODCA0001)
    DomainPresent                               = The domain '{0}' is present. Looking for read only domain controller accounts. (ADRODCA0002)

nit: domain controller account.

E.g., it only looks for one right? 😀

source/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 line 1354 at r5 (raw file):

Previously, Borgquite (Chris Hill) wrote…

Sure.

TL;DR Get-DomainControllerObject is currently broken when ComputerName is not $env:COMPUTERNAME, and this fixes it.

--

Get-DomainControllerObject is used by ADDomainController and now ADReadOnlyDomainControllerAccount to find the domain controller object for a computer. Until this change, it's never been used for anything other than $env:COMPUTERNAME (local node) inside ADDomainController - but I'm reusing it in ADReadOnlyDomainControllerAccount.

However, at present, if ComputerName is set to another name, to check a domain controller other then the local computer, it always fails. This line basically says 'if there is no domain controller account object named COMPUTERNAME, and the local computer is a domain controller, throw an error' (Test-IsDomainController returns true if the local computer is a domain controller). This doesn't make any sense at all if COMPUTERNAME is a remote computer - so Get-DomainControllerObject always fails if $env:COMPUTERNAME != $ComputerName. The fix here is not to run Test-IsDomainController if we're running against another computer - which allows the function to be used in ADReadOnlyDomainController as intended.

That makes sense. Can a new unit test be added for this function so that we can test the behavior is now correct?

tests/Unit/MSFT_ADReadOnlyDomainControllerAccount.Tests.ps1 line 40 at r6 (raw file):

        # Load stub cmdlets and classes.
        Import-Module (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs\ActiveDirectory_2019.psm1') -Force

nit: not something to fix here, but I feel at some point this will need to be determined dynamically (e.g., different stubs needed for 2022 or newer).

tests/Unit/MSFT_ADReadOnlyDomainControllerAccount.Tests.ps1 line 68 at r6 (raw file):

            }

            Context 'When the domain name is not available' {

nit: When the domain could not be found?

tests/Unit/MSFT_ADReadOnlyDomainControllerAccount.Tests.ps1 line 88 at r6 (raw file):

            }

            Context 'When the system is in the desired state' {

nit: Is this level of Context needed? It doesn't really make sense when combined with the subcontexts. Could just remove to reduce the block depth.

source/DSCResources/MSFT_ADReadOnlyDomainControllerAccount/en-US/about_ADReadOnlyDomainControllerAccount.help.txt line 34 at r6 (raw file):

    Specifies if the read only domain controller will be a Global Catalog (GC).

.PARAMETER Ensure

I don't think this should be here.

PlagueHO

Reviewed 1 of 9 files at r4.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @Borgquite and @johlju)

Borgquite

In general, the resource is based on ADDomainController - so a lot of my fixes below I've also fixed there, for consistency. For that reason I also haven't done any integration tests - there aren't any for ADDomainController, and I guess that may be due to complexity (you'd have to spin up an existing domain first to be able to do it, a job beyond me!) However because the resource is a copy/paste of a resource that is working well, I hope we can live without it for now.

Reviewable status: 6 of 17 files reviewed, 15 unresolved discussions (waiting on @johlju and @PlagueHO)

CHANGELOG.md line 23 at r6 (raw file):