The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Bug #17031

Can't add domain user account as a member of a local group

Added by Josh Cooper over 3 years ago. Updated over 2 years ago.

Status:ClosedStart date:
Priority:HighDue date:
Assignee:Josh Cooper% Done:

0%

Category:-
Target version:3.4.0
Affected Puppet version:2.7.6 Branch:https://github.com/puppetlabs/puppet/pull/1931
Keywords:windows user group domain

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


Description

This is a common need when managing domain service accounts that need to be a member of the local Administrators account. I thought it would be resolved once #16581 was fixed, but there’s a more fundamental issue with the group provider, so I’m filing this as a separate issue.

First, it attempts to add members to the group using an ADSI path of WinNT://WIN-QP47VOHA2P4/BIZARRO\albert,user, but it needs to be WinNT://WIN-QP47VOHA2P4/BIZARRO/albert,user

    def add_members(*names)
      names.each do |name|
        native_group.Add(Puppet::Util::ADSI::User.uri(name))
      end
    end

It may be possible to just use the SID form WinNT://<SID> but I’m not sure if that will work in a non-domain environment.

Second, when calculating whether the group’s members are insync? it compares names:

      members_to_add = desired_members - current_members
      add_members(*members_to_add)

However the ADSI provider returns current members as, e.g. albert. But since this doesn’t match BIZARRO\albert, the provider will think the resource is out of sync and will attempt to re-add a user that is already a member of the group and fail:

err: /Stage[main]//Group[Foobars]/members: change from albertAdministrator to BIZARRO\albert Administrator failed: Add
    OLE error code:80070562 in Active Directory
      The specified account name is already a member of the group.

    HRESULT error code:0x80020009
      Exception occurred.

Really, the group provider needs to compare the current vs desired SIDs to determine which users to add, similar to what we do in the file and scheduled_task providers.


Related issues

Related to Puppet - Bug #15326: Scheduled_task does not accept domain user accounts Closed 07/02/2012
Related to Puppet - Bug #16581: Windows file permission management very slow Closed 09/25/2012
Related to Puppet - Bug #19414: Group type should add users to local groups even if the u... In Topic Branch Pending Review
Related to Puppet - Feature #21243: Allow the windows service user to be specified during ins... Closed

History

#2 Updated by Anonymous over 3 years ago

  • Target version deleted (2.7.x)

As the 2.7.x line is winding down, I am removing the target at 2.7.x from tickets in the system. The 2.7 line should only receive fixes for major problems (crashes, for instance) or security problems.

#3 Updated by Mark Garrison almost 3 years ago

  • Target version set to 3.x

Setting the target version in the hope that someone will take an interest in fixing this.

#4 Updated by Josh Cooper almost 3 years ago

Hi Mark, thanks for your interest, it’s very high on our list of windows tickets to fix. The best thing to do is vote on tickets rather than changing the target version, as that is something we use internally.

#5 Updated by Mark Garrison almost 3 years ago

Thanks for the tip Josh, but I can’t see any voting buttons. I’m at the following url http://projects.puppetlabs.com/issues/17031

I tried Chrome, FF and IE (latest versions)

Is there a setting or permission to enable voting?

Josh Cooper wrote:

Hi Mark, thanks for your interest, it’s very high on our list of windows tickets to fix. The best thing to do is vote on tickets rather than changing the target version, as that is something we use internally.

#6 Updated by Josh Cooper almost 3 years ago

  • Target version changed from 3.x to 3.3.0

Hi Mark, thanks for trying, and I gave you bad advice. You used to be able to vote on issues, but due to #20168 only the reporter can vote on the issue, so yeah…

This is an important issue that we hope to get fixed soon.

#7 Updated by eric sorenson almost 3 years ago

  • Assignee set to Josh Cooper

Came across this scrubbing bugs for 3.3.0 — Josh is this something we could get scoped/estimated for work in the next couple of weeks?

Voting should be fixed now BTW.

#8 Updated by eric sorenson over 2 years ago

  • Priority changed from Normal to High
  • Target version changed from 3.3.0 to 3.x

The 3.3.0 ship is sailing and this didn’t get worked on. We’ll bump it up in priority for next release.

#9 Updated by Ethan Brown over 2 years ago

  • To reiterate the obvious, there is a difference between a local user / SID vs a domain user / SID
  • There are a number of available options for binding string syntax, but for practical reasons, the only viable option is to use WinNT:// — additional details follow. Note that as mentioned above, \ is treated as an escape for a literal. Not mentioned in the MSDN docs is an additional GC:// global catalog style binding string to “execute fast queries”
  • WinNT://SID and WinNT://servername/SID are invalid — to find a user by SID, when limited to only WinNT:// style bind paths, enumeration of the local groups / users is required, to perform a match on a given SID — some VB code is available here
  • For clarification, the WinNT://servername/username,user bind syntax is overloaded. servername may either be a host or a domain
  • The syntax that could be used for SID lookup uses LDAP://, but this is not viable.
    • LDAP://COMPUTER/<SID=XXX>, where XXX is a hexadecimal representation of the raw SID byte array is one option. It may also take the form of <SID=S-X-X-XX-XXXXXXXXXX-XXXXXXXXXX-XXXXXXXXX-XXX> where this is a string generated by the ConvertSidToStringSid Win32 API call – see Binding to an Object Using a SID. Also see LDAP ADsPath for the “official” MSDN documentation.
    • Yet another available bind syntax is LDAP://servername/<GUID=XXX> where XXX is either a string of 32 hex characters without punctuation OR a string of hex characters XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX – see Using objectGUID to Bind to an Object for more information.
    • Neither of the LDAP style bind syntax options may be used, as they alway need a host, and in local testing LDAP://./foo and LDAP://localhost/foo do not work by default on machines that are not running a DC. In testing, a PowerShell style query like [ADSI]"LDAP://localhost/<SID=S-1-5-18>" | Select * does not error, but it also does not yield any results. When removing the host, [ADSI]"LDAP://<SID=S-1-5-18>" | Select * does error with exception occurred while retrieving member "distinguishedName": "The specified domain either does not exist or could not be contacted". NOTE: Also attempted were non well-known SIDs as well (users local to a given test environment).
    • Additionally, binding by SID like the above, when it works, will only return the distinguishedName property (based on forum reports). To retrieve the entire AD object would require an additional refetch with an LDAP://distinguishedName type query (for instance, to build a DOMAIN\Username style name).
  • Well-known SIDs like SYSTEM (S-1-5-18), Local Service (S-1-5-19), Network Service (S-1-5-20) are not globally unique and would require a host prefix if used in any sort of comparison.
  • WIN32OLE objects returned by the WinNT syntax with connect method expose an objectSID property that is an array of bytes in octet string format. The IADsUser docs do not mention this as it is specifically a WinNT Custom User Property. This array of bytes cannot be passed directly to the Win32::Security::SID.new we are using as only a binary string format is accepted. Conversion with Ruby’s Array#pack method must be performed
  • So how to translate from a SID to a username when needed?
    • The Win32 LookupAcocuntSid function is automatically called within Win32::Security::SID::new, so opt to use this where possible.
    • There is some sample Powershell code demonstrating the conversion of a SID to a user name. This relies heavily on some .NET Framework classes, particularly SecurityIdentifier which understands how to convert a byte array into a SID. More Powershell examples here
    • Such a PowerShell example looks like (New-Object System.Security.Principal.NTAccount("Administrator")).Translate([System.Security.Principal.SecurityIdentifier]) | Format-List

Ultimately, the end game here is to:

  • Allow a DOMAIN\User style syntax to be expressed properly
  • Prevent the addition of duplicate users to groups, which generates errors

The address this, the following is recommended:

  • Continue to allow group members to be defined in the same manner in the manifest
  • The members method of Puppet::Util::ADSI::Group should not need to be changed to avoid introducing breaking changes to the existing Puppet group type, which uses :array_matching => :all. This will therefore allow duplicate definitions to be expressed in the manifest, like ‘user’ and ‘.\user’, but in practical terms this should not be an issue.
  • That will yield a little extra work to lookup SIDs in the set_members, add_members, and remove_members methods of Puppet::Util::ADSI::Group#add_members
    • These methods can use the existing Puppet::Util::Windows::SID.name_to_sid helper that leverages Win32::Security::SID.new to lookup the SID for the specified string names. This SID value can be used to compare against the objectSID property of each Puppet::Util::ADSI::User in native_group.Members
  • I’m not certain if there is any value to implementing an insync? method inside of lib\puppet\provider\group\windows_asdi.rb, since this would make the SID lookup / matching occur twice? Does this change how anything is reported?

#10 Updated by Rob Reynolds over 2 years ago

Ethan Brown wrote:

* Neither of the `LDAP` style bind syntax options may be used, as they alway need a host, and in local testing `LDAP://./foo` and `LDAP://localhost/foo` do not work by default on machines that are not running a DC.  In testing, a PowerShell style query like `[ADSI]"LDAP://localhost/<SID=S-1-5-18>" | Select *` does not error, but it also does not yield any results.  When removing the host, `[ADSI]"LDAP://<SID=S-1-5-18>" | Select *` does error with `exception occurred while retrieving member "distinguishedName": "The specified domain either does not exist or could not be contacted"`.  NOTE: Also attempted were non well-known SIDs as well (users local to a given test environment).

Did you try LDAP://machineName/foo ?

#11 Updated by Rob Reynolds over 2 years ago

Ethan Brown wrote:

  • Well-known SIDs like SYSTEM (S-1-5-18), Local Service (S-1-5-19), Network Service (S-1-5-20) are not globally unique and would require a host prefix if used in any sort of comparison.

Do you have a reference? This is a pretty scary proposition if true.

I agree if the wording is the distinguished name / domain of the well known SIDs are not unique with every OS version (sometimes BUILTIN\SYSTEM sometimes NT AUTHORITY\SYSTEM, etc).

#12 Updated by Rob Reynolds over 2 years ago

The ideas in the end game seem sound (from someone outside of the current problem set).

#13 Updated by Ethan Brown over 2 years ago

Rob Reynolds wrote:

Ethan Brown wrote:

  • Well-known SIDs like SYSTEM (S-1-5-18), Local Service (S-1-5-19), Network Service (S-1-5-20) are not globally unique and would require a host prefix if used in any sort of comparison.

Do you have a reference? This is a pretty scary proposition if true.

I agree if the wording is the distinguished name / domain of the well known SIDs are not unique with every OS version (sometimes BUILTIN\SYSTEM sometimes NT AUTHORITY\SYSTEM, etc).

I’m not even certain if the situation I’m trying to bring to light is worth mentioning, and I don’t have a domain handy to verify if it’s an issue. My point here is that there could be a collision if the objectSID property of the WIN32OLE instance only returns a well-known SID like S-1-5-19. If a manifest tried to add HOST1\Network Service and HOST2\Network Service, I believe their objectSID values are the same, and only the first would be added. So in this case, the host is necessary for the comparison. I can’t think of any reason that someone would want to add well-known accounts from two separate hosts to a local group. Keep in mind though that groups may contain groups so it’s not implausible to add the domain admin groups from two separate domains to the local admin group, which might create the same issue.

#14 Updated by Ethan Brown over 2 years ago

Rob Reynolds wrote:

Ethan Brown wrote:

* Neither of the `LDAP` style bind syntax options may be used, as they alway need a host, and in local testing `LDAP://./foo` and `LDAP://localhost/foo` do not work by default on machines that are not running a DC.  In testing, a PowerShell style query like `[ADSI]"LDAP://localhost/<SID=S-1-5-18>" | Select *` does not error, but it also does not yield any results.  When removing the host, `[ADSI]"LDAP://<SID=S-1-5-18>" | Select *` does error with `exception occurred while retrieving member "distinguishedName": "The specified domain either does not exist or could not be contacted"`.  NOTE: Also attempted were non well-known SIDs as well (users local to a given test environment).

Did you try LDAP://machineName/foo ?

Yes. The result is the same.

#15 Updated by Josh Cooper over 2 years ago

  • Status changed from Accepted to Merged - Pending Release
  • Target version changed from 3.x to 3.4.0
  • Branch set to https://github.com/puppetlabs/puppet/pull/1931

Merged into master in commit fc3faee, to be released in 3.4.0

#16 Updated by Melissa Stone over 2 years ago

  • Status changed from Merged - Pending Release to Closed

Released in Puppet 3.4.0-rc1

#17 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#18 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#19 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#20 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#21 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

Also available in: Atom PDF