Bug #5608

Puppet shouldn't enumerate LDAP users for local user unmanaged resource purge

Added by Sean Millichamp over 1 year ago. Updated 4 months ago.

Status:Code Insufficient Start date:12/19/2010
Priority:Normal Due date:
Assignee:Sean Millichamp % Done:

100%

Category:user
Target version:2.7.x
Affected Puppet version:2.6.4 Branch:https://github.com/seanmil/puppet/tree/ticket/2.6.x/5608
Keywords:
Votes: 3

Description

When using:

resources { ‘user’: purge => true }

in a Puppet configuration not setup for LDAP management (intentionally) it is using the system getent functions via listbyname() (inherited from lib/puppet/provider/nameservice.rb) which nevertheless lists all the LDAP users because they show in the getent database via nsswitch.

This causes a number of problems in my situation:

1) The LDAP tree is large enough that Puppet can’t complete in a reasonable amount of time when it has to list all of the users in LDAP 2) Puppet will see users it can’t delete 3) Even if it could delete those users, I only want to use Puppet to manage just the local users

Based on my reading of the code, if Puppet is being used to manage LDAP users the ldap.rb provider manages that itself and doesn’t require use of getpwent in nameservice.rb

The workaround I used is by overriding the listbyname() function in a custom provider (which inherits from useradd) to look for users in /etc/passwd. It seems like it would be safe to just modify the listbyname() function in nameservice.rb to look directly in /etc/passwd but I am not certain what else that might impact.

History

Updated by James Turnbull over 1 year ago

  • Status changed from Unreviewed to Investigating

Sean – I’d submit a patch modifying the function and we’ll do some testing.

Thanks

Updated by Sean Millichamp over 1 year ago

  • Branch set to https://github.com/seanmil/puppet/tree/ticket/2.6.x/5608

I have sent a patch to the puppet-dev list, pushed the branch to github, and updated the Branch field in this ticket to refer to it.

I didn’t write any unit tests for it since I wasn’t sure how to approach a unit test for this case. If a test is required and you have some ideas on what it should look like I can try to tackle it.

Thanks!

Updated by Sean Millichamp over 1 year ago

  • Status changed from Investigating to In Topic Branch Pending Review

Updated by Sean Millichamp about 1 year ago

  • Assignee set to Sean Millichamp
  • % Done changed from 0 to 100

I received some positive feedback on the patch when I posted it to the list a couple of months ago, but the ticket has sat without any action.

I’d really like to see this get into an near upcoming release of Puppet as it causes real issues for me – what next steps do I need to be taking on this?

Thanks!

Updated by Jesse Wolfe about 1 year ago

Sorry, we’ve got a backlog of submitted patches that we’re trying to work through. I’ve commented on the mailing list – let’s have a discussion there.

Updated by James Turnbull about 1 year ago

  • Target version set to 2.6.x

Updated by Jesse Wolfe about 1 year ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient

As is, I think this patch isn’t quite ready. There are two lingering concerns:

1) If this is the solution we decide to pursue, we should make it work for both users and groups. I suspect that means the code can go in their common implementation ancestor in lib/puppet/provider/nameservice/objectadd.rb

2) There’s a design question about whether parsing files in /etc/ is really “correct” – it might make more sense to supplement the information returned by getent with metadata about whether the user is locally deletable

Updated by Daniel Pittman about 1 year ago

Jesse Wolfe wrote: […]

2) There’s a design question about whether parsing files in /etc/ is really “correct” – it might make more sense to supplement the information returned by getent with metadata about whether the user is locally deletable

Other than total guesswork, it isn’t actually possible for us to determine that ahead of time, only when we try and perform the operation.

For example, it would be possible for some-but-not-all of the Active Directory integrated systems I ran to delete somewhat varying sets of users “remotely” using the appropriate local tools, at a site a few years back. (Specifically, different machines in different “divisions” had different security rules; our adduser and deluser tools were configured appropriately to be able to delete things in the AD data store.)

The same is true of LDAP, NIS, and various other unconventional data stores. (eg: pam-mysql could totally replace /etc/passwd, but entirely preserve the ability to delete users using the same API.)

So, I would suggest that this is impossible to satisfy except in limited circumstances, and doing so would require bypassing the NSS service entirely to determine that metadata. Plus, on a non-trivial number of platforms we don’t even have that rich a view of providers. (eg: Win32).

If we do accept something like this, it would need to be done in a way that it was optional, and non-default, to ensure that we don’t stop working for configurations that don’t match that specific set of assumptions.

Updated by Michael Stahnke 4 months ago

  • Target version changed from 2.6.x to 2.7.x

2.6.x is closed. Moving to 2.7.x

Also available in: Atom PDF