Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADGroup: Group Members From One-Way Trusted Domain Break Functionality #616

Closed
jeremyciak opened this issue Jun 24, 2020 · 29 comments · Fixed by #617
Closed

ADGroup: Group Members From One-Way Trusted Domain Break Functionality #616

jeremyciak opened this issue Jun 24, 2020 · 29 comments · Fixed by #617
Labels
bug The issue is a bug. help wanted The issue is up for grabs for anyone in the community.

Comments

@jeremyciak
Copy link
Contributor

jeremyciak commented Jun 24, 2020

Details of the scenario you tried and the problem that is occurring

The scenario is that we have 2 domains "Domain A" and "Domain B" where a one-way trust is established so that it is outgoing from Domain A and incoming for Domain B. This setup ensures that users from Domain B are trusted by Domain A. If I am trying to manage AD groups in Domain A where any resources from Domain B have been set as members, the usage of the Get-ADGroupMember cmdlet in the ADGroup resource fails spectacularly in the strangest way.

Verbose logs showing the problem

The server was unable to process the request due to an internal error. 
For more information about the error, either turn on IncludeExceptionDetailInFaults
(either from ServiceBehaviorAttribute or from the <serviceDebug> configuration behavior) on the server in order to send
the exception information back to the client, or turn on tracing as per the Microsoft .NET Framework SDK documentation
and inspect the server trace logs.

Suggested solution to the issue

Instead of using the Get-ADGroupMember cmdlet we could do the following:

(Get-ADGroup -Identity <GroupID> -Properties Members).Members | 
    ForEach-Object -Process { 
        Get-ADObject -Identity $_ -Properties SamAccountName, ObjectSID |
            Select-Object -Property distinguishedName, name, objectClass, objectGUID, SamAccountName, @{ Name = 'SID'; Expression = {$_.ObjectSID} }
    }

The DSC configuration that is used to reproduce the issue (as detailed as possible)

ADGroup 'TestGroup'
{
            GroupName   = 'TestGroup'
            GroupScope  = 'DomainLocal'
            Category    = 'Security'
            Ensure      = 'Present'
            Path        = "CN=Users,DC=DomainB,DC=local"
            Description = 'Test Group Description'
}

The operating system the target node is running

OsName               : Microsoft Windows Server 2019 Datacenter
OsOperatingSystemSKU : DatacenterServerEdition
OsArchitecture       : 64-bit
WindowsVersion       : 1809
WindowsBuildLabEx    : 17763.1.amd64fre.rs5_release.180914-1434
OsLanguage           : en-US
OsMuiLanguages       : {en-US}

Version and build of PowerShell the target node is running

Name                           Value                                                                                                                                                                                                
----                           -----                                                                                                                                                                                                
PSVersion                      5.1.17763.1007                                                                                                                                                                                       
PSEdition                      Desktop                                                                                                                                                                                              
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}                                                                                                                                                                              
BuildVersion                   10.0.17763.1007                                                                                                                                                                                      
CLRVersion                     4.0.30319.42000                                                                                                                                                                                      
WSManStackVersion              3.0                                                                                                                                                                                                  
PSRemotingProtocolVersion      2.3                                                                                                                                                                                                  
SerializationVersion           1.1.0.1

Version of the DSC module that was used

6.0.1

@jeremyciak
Copy link
Contributor Author

#617

@davidwallis
Copy link

are you able to create the group memberships across a trust? I am not managing it currently.. I have two forests and a one way trust and trying to add a computer to a domain local group as a test but it always try to look up the account in the wrong domain, even when I use the SID for the computer. (I have working powershell for the adding now by using the sid..) but would rather do this with DSC.

@jeremyciak
Copy link
Contributor Author

are you able to create the group memberships across a trust? I am not managing it currently.. I have two forests and a one way trust and trying to add a computer to a domain local group as a test but it always try to look up the account in the wrong domain, even when I use the SID for the computer. (I have working powershell for the adding now by using the sid..) but would rather do this with DSC.

I am not even to the point where I am attempting to manage the group membership, just the group creation and details. I am managing the domain on the side of the trust where I do not have permissions into the other domain so I would not be able to manage membership. However, after my DSC runs and creates the groups and once any resources from the other domain are added as members then every subsequent DSC run fails with the error stated above.

My proposal is attempting to update the code so that there are no breaking changes or really any added functionality, it should just allow my situation to not fail and keep everyone else humming along.

@X-Guardian
Copy link
Contributor

Hi @jeremyciak, thanks for reporting this, but before you spend any more time on the PR, can you please edit the issue description and add the DSC configuration you are using so we can better understand the problem.

@jeremyciak
Copy link
Contributor Author

Hey @X-Guardian, this has been done. The main thing to note is that I do not care at all about the group membership, but because there are members from another domain in the groups I am trying to manage, this DSC resource breaks down due to some issue with the Get-ADGroupMember command.

@jeremyciak
Copy link
Contributor Author

I'm excited that I was able to get my PR to pass and I'm curious what the next steps are to proceed further from here.

@X-Guardian
Copy link
Contributor

HI @jeremyciak, I have reproduced the error with Get-ADGroupMember with a one way external trust as you describe. With a two way external trust, Get-ADGroupMember successfully returns the properties of the same group. Unfortunately, with your suggested solution, both with a one way trust and and a two way trust, the SamAccountName is not returned from Get-AdObject, and the objectClass is returned as foreignSecurityPrincipal rather than user.
This would therefore break the resource functionality for the more common scenario usage with two way trusts.

@johlju johlju added bug The issue is a bug. help wanted The issue is up for grabs for anyone in the community. labels Jun 25, 2020
@jeremyciak
Copy link
Contributor Author

Would you be open to a try/catch solution?

@X-Guardian
Copy link
Contributor

Expand on what you had in mind, preferably with a code snippet to try.

@jeremyciak
Copy link
Contributor Author

jeremyciak commented Jun 25, 2020

From my PR... we could replace this:

$adGroupMembers = (Get-ADGroupMember @commonParameters).$MembershipAttribute

... with this

try
{
    $adGroupMembers = (Get-ADGroupMember @commonParameters).$MembershipAttribute
}
catch
{
    $oneWayTrustErrorMessage = "The server was unable to process the request due to an internal error.  For more information about the error, either turn on IncludeExceptionDetailInFaults (either from ServiceBehaviorAttribute or from the <serviceDebug> configuration behavior) on the server in order to send the exception information back to the client, or turn on tracing as per the Microsoft .NET Framework SDK documentation and inspect the server trace logs."

    if ($_.Exception.Message -eq $oneWayTrustErrorMessage)
    {
        # Get-ADGroupMember returns property name 'SID' while Get-ADObject returns property name 'ObjectSID'
        if ($MembershipAttribute -eq 'SID')
        {
            $selectProperty = 'ObjectSID'
        }
        else
        {
            $selectProperty = $MembershipAttribute
        }

        # Use the same results from Get-ADCommonParameters but remove the Identity for usage with Get-ADObject
        $commonParametersClone = $commonParameters.Clone()
        $null = $commonParametersClone.Remove('Identity')

        # This creates a filter which multiple -or statements matching the various DistinguishedName values of the Members
        # This allows for a single Get-ADObject call instead of multiple calls in a loop
        $adObjectFilter = @(
            "(DistinguishedName -eq '",
            ((Get-ADGroup @commonParameters -Properties Members).Members -join "') -or (DistinguishedName -eq '"),
            "')"
        ) -join ''

        # Retrieve the current list of members, returning the specified membership attribute
        $adGroupMembers = Get-ADObject -Filter $adObjectFilter -Properties 'SamAccountName', 'ObjectSID' @commonParametersClone |
            Select-Object -ExpandProperty $selectProperty
    }
    else
    {
        Write-Error -Exception $_.Exception
    }
}

This would make it so that my changes only get executed to address the specific exception message thrown for this specific one-way trust scenario.

@davidwallis
Copy link

I'll post it here for reference - but I'm in a similar situation with the one way trust - i've got some code that works as a crude test for adding a computer account from an one way forest trust into domain local group - but I haven't yet worked out how it could be made to fit with the current implementation..

This is the only way I've managed to add the group membership so far - obv the dscresource handles the contexts and disposing better than this - but it would require multiple contexts for the trusted domain. I'm all ears if there's a neater way?

again this is just from my testing - not what I expect as a solution - its just taken me a lot of testing to finally get something to add the group member.

  Node localhost {
        Script ConfigureGroups {
            SetScript = {
                Add-Type -AssemblyName System.DirectoryServices.AccountManagement
                $groupName ="testGroup"
                $computerName = "Server1"

    
                $pc = New-Object System.DirectoryServices.AccountManagement.PrincipalContext(
                    [System.DirectoryServices.AccountManagement.ContextType]::Domain, "svc.local", "install", "password"
                )

        
                $group = [System.DirectoryServices.AccountManagement.GroupPrincipal]::FindByIdentity($pc, [System.DirectoryServices.AccountManagement.IdentityType]::Name, $groupName)
                $groupDirectoryEntry = $Group.GetUnderlyingObject()

                $remoteCtx = New-Object System.DirectoryServices.AccountManagement.PrincipalContext(
                    [System.DirectoryServices.AccountManagement.ContextType]::Domain, "db.local", "install", "password"
                )
               

                $computer = [System.DirectoryServices.AccountManagement.ComputerPrincipal]::FindByIdentity($remoteCtx, [System.DirectoryServices.AccountManagement.IdentityType]::Name, $computerName)

                write-verbose "Adding computer by sid"
                $groupDirectoryEntry.Properties["member"].Add("<SID=$($computer.Sid.Value)>") |out-Null
                $groupDirectoryEntry.CommitChanges()

                if ($null -ne $pc) { $pc.Dispose() }
                if ($null -ne $remoteCtx) { $remoteCtx.Dispose() }
            }
    }
}

David

@johlju
Copy link
Member

johlju commented Jun 26, 2020

This would make it so that my changes only get executed to address the specific exception message thrown for this specific one-way trust scenario.

This would be to error prone when running on a non-english OS. Secondly I'm not in favor of having so much code inside of catch block. 🤔 Isn't there a better way to handle this? 🤔

@johlju
Copy link
Member

johlju commented Jun 26, 2020

Is it possible to determine if the domain have this one-way trust relationship and change the the logic accordingly? How would that look like? 🤔

@johlju
Copy link
Member

johlju commented Jun 26, 2020

@jeremyciak would @davidwallis3101 code example help in any way? It uses a different logic to find members?

@jeremyciak
Copy link
Contributor Author

@johlju,

The issue with the implementation from @davidwallis3101 is that it utilizes C# and would not allow us to benefit easily from the $commonParameters splat when someone specifies DomainController or Credential parameters.

Another method I was thinking is that if there is nothing specified for Members, IncludeMembers, or ExcludeMembers then we don't do any member logic. I will look into using that method instead.

@johlju
Copy link
Member

johlju commented Jun 26, 2020

Another method I was thinking is that if there is nothing specified for Members, IncludeMembers, or ExcludeMembers then we don't do any member logic. I will look into using that method instead.

Yes, set should not enforce any Member parameters if they are not specified in the configuration, although the Get-function should return the Members regardless.

@jeremyciak
Copy link
Contributor Author

I just realized my above suggestion would break what was done for #189 which wants to blank out membership when nothing is specified.

Would you be open to adding a (switch or boolean) parameter to the DSC Resource for "IgnoreMembership" that allows for a situation like I have where I just want to create groups so that I can assign and manage permissions but I want to delegate to other people to control membership to the group?

@jeremyciak
Copy link
Contributor Author

My PR #617 has been updated to show a possible implementation of this new property.

@davidwallis
Copy link

davidwallis commented Jun 27, 2020

@jeremyciak why would the sample code I've provided be an issue - contexts are already used, and an implementation is provided here:

surely additional optional parameters could/can be passed to the Add-ADCommonGroupMember function? for the relevant credential objects and domain names (if not using objectDN)

@X-Guardian
Copy link
Contributor

Hi @davidwallis3101, using this piece of your code:

Add-Type -AssemblyName System.DirectoryServices.AccountManagement
$domain1Name = 'gteck.com'
$groupName = 'test-domainlocal-group'
    
$pc = New-Object System.DirectoryServices.AccountManagement.PrincipalContext(
    [System.DirectoryServices.AccountManagement.ContextType]::Domain, $domain1Name
)
        
$group = [System.DirectoryServices.AccountManagement.GroupPrincipal]::FindByIdentity($pc,
    [System.DirectoryServices.AccountManagement.IdentityType]::Name, $groupName)
$group.Members

I was able to get a list of group members from a group which contained a foreign security principal from a domain across a one way external trust. The member object had correctly populated SamAccountName, UserPrincipalName, DistinguishedName and SID properties. @jeremyciak, can you verify that you get the same results?

I would suggest therefore, using these .Net Classes (this is not C# @jeremyciak) is the way forward, and first step would be to modify the Get-TargetResource function to use this, which would solve @jeremyciak's problem. We can then look at modifying the Set-TargetResource function afterwards.

To try and avoid breaking any current working scenarios, let's start by only using this code if the current Get-ADGroupMember cmdlet fails. It looks like a check for $_.Exception.FullyQualifiedErrorId containing ActiveDirectoryServer:0,Microsoft.ActiveDirectory.Management.Commands.GetADGroupMember in a try/catch block would work.

@jeremyciak
Copy link
Contributor Author

@X-Guardian, unfortunately I am not getting the same results. Here is my code:

$groupName = 'GroupWithForeignSecurityPrincipal'

try {
    Get-ADGroupMember -Identity $groupName
}
catch {
    if ($_.FullyQualifiedErrorId -eq 'ActiveDirectoryServer:0,Microsoft.ActiveDirectory.Management.Commands.GetADGroupMember') {
        Add-Type -AssemblyName System.DirectoryServices.AccountManagement
        $domainName = [System.DirectoryServices.ActiveDirectory.Domain]::GetCurrentDomain().Name
        
        $principalContext = [System.DirectoryServices.AccountManagement.PrincipalContext]::new(
            [System.DirectoryServices.AccountManagement.ContextType]::Domain,
            $domainName
        )
        
        $group = [System.DirectoryServices.AccountManagement.GroupPrincipal]::FindByIdentity(
            $principalContext,
            [System.DirectoryServices.AccountManagement.IdentityType]::Name,
            $groupName
        )

        $group.Members
    }
    else {
        Write-Error -Exception $_.Exception
    }
}

I get this error:
An error occurred while enumerating through a collection: The user name or password is incorrect.

When I run the same code against a group with no foreign security principals it returns results just fine.

@jeremyciak
Copy link
Contributor Author

jeremyciak commented Jun 30, 2020

How about this:

try
{
    $adGroupMembers = (Get-ADGroupMember @commonParameters).$MembershipAttribute
}
catch
{
    if ($_.FullyQualifiedErrorId -eq 'ActiveDirectoryServer:0,Microsoft.ActiveDirectory.Management.Commands.GetADGroupMember')
    {
        # Get-ADGroupMember returns property name 'SID' while Get-ADObject returns property name 'ObjectSID'
        if ($MembershipAttribute -eq 'SID')
        {
            $selectProperty = 'ObjectSID'
        }
        else
        {
            $selectProperty = $MembershipAttribute
        }

        # Use the same results from Get-ADCommonParameters but remove the Identity for usage with Get-ADObject
        $commonParametersClone = $commonParameters.Clone()
        $null = $commonParametersClone.Remove('Identity')

        # This creates a filter which multiple -or statements matching the various DistinguishedName values of the Members
        # This allows for a single Get-ADObject call instead of multiple calls in a loop
        $adObjectFilter = @(
            "(DistinguishedName -eq '",
            ((Get-ADGroup @commonParameters -Properties Members).Members -join "') -or (DistinguishedName -eq '"),
            "')"
        ) -join ''

        # Retrieve the current list of members, returning the specified membership attribute
        $adGroupMembers = Get-ADObject -Filter $adObjectFilter -Properties 'SamAccountName', 'ObjectSID' @commonParametersClone | ForEach-Object -Process {
            if (($_.objectClass -eq 'foreignSecurityPrincipal') -and ($MembershipAttribute -eq 'SamAccountName'))
            {
                [System.Security.Principal.SecurityIdentifier]::new($_.objectSid).Translate([System.Security.Principal.NTAccount])
            }
            else
            {
                $_.$selectProperty
            }
        }
    }
    else
    {
        Write-Error -Exception $_.Exception
    }
}

The main thing to note here is the usage of [System.Security.Principal.SecurityIdentifier]::new($_.objectSid).Translate([System.Security.Principal.NTAccount]) to resolve the SamAccountName for the members with objectClass of "foreignSecurityPrincipal".

I created a quick little function Test-GroupStuff so that you can iterate through some groups and the various MembershipAttribute options and see the results in whatever test domain you have access to:

function Test-GroupStuff {
    [CmdletBinding()]
    param(
        [Parameter()]
        [System.String]
        $GroupName,

        [Parameter()]
        [ValidateSet('SamAccountName', 'DistinguishedName', 'SID', 'ObjectGUID')]
        [System.String]
        $MembershipAttribute = 'SamAccountName'
    )

    $commonParameters = @{
        Identity = $groupName
    }

    try
    {
        $adGroupMembers = (Get-ADGroupMember @commonParameters).$MembershipAttribute
    }
    catch
    {
        if ($_.FullyQualifiedErrorId -eq 'ActiveDirectoryServer:0,Microsoft.ActiveDirectory.Management.Commands.GetADGroupMember')
        {
            # Get-ADGroupMember returns property name 'SID' while Get-ADObject returns property name 'ObjectSID'
            if ($MembershipAttribute -eq 'SID')
            {
                $selectProperty = 'ObjectSID'
            }
            else
            {
                $selectProperty = $MembershipAttribute
            }

            # Use the same results from Get-ADCommonParameters but remove the Identity for usage with Get-ADObject
            $commonParametersClone = $commonParameters.Clone()
            $null = $commonParametersClone.Remove('Identity')

            # This creates a filter which multiple -or statements matching the various DistinguishedName values of the Members
            # This allows for a single Get-ADObject call instead of multiple calls in a loop
            $adObjectFilter = @(
                "(DistinguishedName -eq '",
                ((Get-ADGroup @commonParameters -Properties Members).Members -join "') -or (DistinguishedName -eq '"),
                "')"
            ) -join ''

            # Retrieve the current list of members, returning the specified membership attribute
            $adGroupMembers = Get-ADObject -Filter $adObjectFilter -Properties 'SamAccountName', 'ObjectSID' @commonParametersClone | ForEach-Object -Process {
                if (($_.objectClass -eq 'foreignSecurityPrincipal') -and ($MembershipAttribute -eq 'SamAccountName'))
                {
                    [System.Security.Principal.SecurityIdentifier]::new($_.objectSid).Translate([System.Security.Principal.NTAccount])
                }
                else
                {
                    $_.$selectProperty
                }
            }
        }
        else
        {
            Write-Error -Exception $_.Exception
        }
    }

    $adGroupMembers
}

Here is how I was listing some groups to test it out:

$groupNames = @(
    'TestGroupA'
    ,'TestGroupB'
)

foreach ($membershipAttribute in @('SamAccountName', 'DistinguishedName', 'SID', 'ObjectGUID')) {
    foreach ($groupName in $groupNames) {
        Write-Verbose -Message "Retrieving '$($membershipAttribute)' value for each member of group '$($groupName)'" -Verbose
        
        Test-GroupStuff -GroupName $groupName -MembershipAttribute $membershipAttribute | ForEach-Object -Process {
            Write-Verbose -Message "  $($_)" -Verbose
        }
    }
}

@davidwallis
Copy link

davidwallis commented Jul 1, 2020

Its potentially not working due to the context not being authenticated. The constructor takes two additional arguments for username and password..

$principalContext = [System.DirectoryServices.AccountManagement.PrincipalContext]::new(
         [System.DirectoryServices.AccountManagement.ContextType]::Domain,
         $domainName, $username, $password
     )

In my testing I never got it to work quite like that - but I was putting it down to my test env and intented to revisit that later - Looking at other code that's using the context if the credential object isnt passed then the constructor looks like

$principalContext = [System.DirectoryServices.AccountManagement.PrincipalContext]::new(
         [System.DirectoryServices.AccountManagement.ContextType]::Domain,
         $domainName, $null, $null
     )

I doubt that would make a difference but I've not looked and the underlying code, I'll have a play around today with this.

@davidwallis
Copy link

@jeremyciak that example code works for me however I foresee a problem when when using this as the test as when you return the DN for the member you are returning the fsp DN

however that isn't what someone is going to specify as part of member(s) or members to include - so something would need to be done with this, At the moment the only way I have managed to get the member to populate was to use the SID - however I think it would be better to translate that at the time of adding so that the dsc would be able to create the object and add it, without having to know the SID.

But this example does have the benefits (on my test env) that I haven't had to provide any permissions above those that I am testing with.

@jeremyciak
Copy link
Contributor Author

@davidwallis3101, in my testing the DistinguishedName resolves correctly in a two-way trust but in a one-way trust it does represent the foreign security principal SID as you stated. I don't know what can be done to address that behavior for a one-way trust.

@jeremyciak
Copy link
Contributor Author

@X-Guardian @johlju
Any thoughts/comments on the latest proposal?

@davidwallis
Copy link

I could do with some movement in this as I really need the set part :) happy to work on a PR once we know how the get should look and work out whats acceptable for the additional creds etc

@X-Guardian
Copy link
Contributor

OK guys, I've tested @jeremyciak 's Test-GroupStuff function, and it works for me, and it hopefully gives us a route forward. I'll now review PR #617.

@briantist
Copy link

This is awesome. I've been hoping for a fix to this for so long #235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug. help wanted The issue is up for grabs for anyone in the community.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants