[PUP-6586] User resource always reports password changed when account disabled Created: 2016/08/04  Updated: 2018/04/03  Resolved: 2016/08/24

Status: Closed
Project: Puppet
Component/s: Windows
Affects Version/s: PUP 4.5.3
Fix Version/s: PUP 4.6.1

Type: Bug Priority: Normal
Reporter: Adam Bottchen Assignee: Rob Reynolds
Resolution: Fixed Votes: 0
Labels: customer-escalation
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: PNG File verification.png    
Issue Links:
Blocks
is blocked by PUP-6618 type: user, fails in macOS with UTF-8... Closed
Relates
relates to PUP-6483 "puppet resource user", when run in W... Closed
relates to PUP-6569 Improve error messaging for Windows u... Resolved
Template:
Acceptance Criteria:

Make sure that we don't report a password change when the account is disabled when managing the password.

Story Points: 2
Sprint: Windows 2016-08-24
Method Found: Customer Feedback
Release Notes: Bug Fix
Release Notes Summary: Previously, when Puppet is managing accounts that are disabled on Windows, it would attempt to change the password every time as it didn't recognize that the logon failure was due to the account being disabled. This causes both reporting and event log messages surrounding these changes. Now Puppet recognizes the account is disabled and no longer makes attempts to change the password while the account is not enabled.

 Description   

On Windows 2008R2, when managing a local user the password will always report as changed if the user's account is disabled. This appears to be due to the provider not checking for the ERROR_ACCOUNT_DISABLED return code as outlined in PUP-6569.

To recreate:

1. Create a new local user, "testuser", on Win 2k8R2 (also seen on 2012) and set the account to disabled.
2. Run `puppet apply -e "user

{'testuser': ensure => present, password => 'Puppet11'}

"`. Note that no matter how many times it is run, the output will report "Notice: /Stage[main]/Main/User[testuser]/password: changed password".
3. Re-enable the account
4. Run `puppet apply -e "user

{'testuser': ensure => present, password => 'Puppet11'}

"`. Note that the notice is no longer shown.



 Comments   
Comment by Moses Mendoza [ 2016/08/04 ]

reproduced in puppet/stable as of 70426ea1effce8aaa545db911c3f340c09c25f56

Comment by Ethan Brown [ 2016/08/04 ]

Due to the way that passwords are checked with the LogonUser API, an account has to be in an enabled state for Puppet to successfully login. Based on a successful login, it knows the current password is correct - but it currently only uses success as an indicator.

So after a quick test, it looks like we have a pretty good option. ERROR_ACCOUNT_DISABLED - 1331 (from PUP-6569) is actually indicative of a successful password check, but failed logon due to disabled account. A quick test seems to indicate that is does indicate a good password. Given disabled account bob with password password:

[2] pry(main)> Puppet::Util::Windows::User.logon_user('bob', 'password')
Puppet::Util::Windows::Error: Failed to logon user "bob":  Logon failure: account currently disabled.
from C:/source/puppet/lib/puppet/util/windows/user.rb:70:in `block in logon_user'
 
[3] pry(main)> Puppet::Util::Windows::User.logon_user('bob', 'bobo')
Puppet::Util::Windows::Error: Failed to logon user "bob":  Logon failure: unknown user name or bad password.
from C:/source/puppet/lib/puppet/util/windows/user.rb:70:in `block in logon_user'

This would be the simplest fix for us.

Other options for the sake of discussion if for some reason the above doesn't work out:

  • Automatically enable the account, verify password like we do now, set back to disabled - I think we can agree this is a horrific idea
  • Silently ignore the given password (and perhaps introduce a warning that passwords for disabled accounts are not managed), only setting the password when the account is actually enabled
  • Require that passwords be not set / set to undef for disabled accounts
  • Find a mechanism other than LogonUser to do the password validation (not sure one exists)
Comment by Moses Mendoza [ 2016/08/05 ]
  • Silently ignore the given password (and perhaps introduce a warning that passwords for disabled accounts are not managed), only setting the password when the account is actually enabled

In this option would we (can we) first do a check for if an account is disabled before actually attempting the logon? If so, I think this may also be one to really consider - maybe it depends on how much it matters that we generate failed login attempts in the security log. This option won't generate spurious/recurring logon failures in the security log (which e.g., potentially obscure our user's audit(s), security measures, etc).

Comment by Ethan Brown [ 2016/08/05 ]

We can indeed verify disabled status on the ADSI User with the AccountDisabled property.

Of course, we're hoping to move away from ADSI in the future to something based more on the Windows API - but that's still a bit out.

Comment by Moses Mendoza [ 2016/08/10 ]
  • Silently ignore the given password (and perhaps introduce a warning that passwords for disabled accounts are not managed), only setting the password when the account is actually enabled

I think this may not be the best path after all - I think we should still do the password validation even when the account is disabled. Their use-case and the spirit of the ticket is that they want to make sure the password is managed even though the account is disabled. We're not really managing the password if we check and skip the validation if the account is disabled. I can see a world where the account is disabled, someone (outside of puppet) changes the password, and then later a walmart admin enables the account, tries to log in, and the password isn't the one puppet was "managing" - at least until puppet runs again. This would be quite surprising.

I think the best path is to validate the password and handle that error so that we don't try the reset.

Comment by Moses Mendoza [ 2016/08/12 ]

WIP pushed https://github.com/MosesMendoza/puppet/commit/6f91c87de98f6884f39cf32f7cd9c37980815541. still needs tests

Comment by Ethan Brown [ 2016/08/19 ]

Merged to stable in https://github.com/puppetlabs/puppet/commit/352dde3557a8ca1854f411501fefda7c3ffcd40e

Comment by Glenn Sarti [ 2016/08/19 ]

Merged to master at;
https://github.com/puppetlabs/puppet/commit/a0516659fc55a614984ed74cf68857aee9497d78

Comment by Ethan Brown [ 2016/08/20 ]

Stable has passed through the Jenkins suite at https://jenkins.puppetlabs.com/view/puppet-agent/view/stable/view/puppet-agent/job/platform_puppet-agent_intn-van-sys_suite-daily-puppet-stable/182/ against all platforms

That CI run contained puppet-agent SHA ad760e51b586b6e0a6dc919610f330ef3a3b76f8 - https://github.com/puppetlabs/puppet-agent/commit/ad760e51b586b6e0a6dc919610f330ef3a3b76f8

Which includes Puppet SHA - 4e7bbb02b80f382011d4e63d40d9292ad7a4614f - https://github.com/puppetlabs/puppet/commits/4e7bbb02b80f382011d4e63d40d9292ad7a4614f

Similarly it passed through the full puppet suite at https://jenkins.puppetlabs.com/view/puppet-agent/view/stable/view/puppet/job/platform_puppet_pkg-van-promote_stable/178/ based on the same Puppet SHA 4e7bbb02b80f382011d4e63d40d9292ad7a4614f

This includes specs run against Ruby 2.3 - https://jenkins.puppetlabs.com/view/puppet-agent/view/stable/view/puppet/job/platform_puppet_unit-ruby-win_stable/

SHA a0516659fc55a614984ed74cf68857aee9497d78 of Puppet - at
https://github.com/puppetlabs/puppet/commits/a0516659fc55a614984ed74cf68857aee9497d78 - has also run to success through the puppet master pipelines at https://jenkins.puppetlabs.com/view/puppet-agent/view/master/view/puppet/job/platform_puppet_pkg-van-promote_master/291/

This is ready for any additional FR

Comment by Kenn Hussey [ 2016/08/24 ]

Ethan Brown this issue needs release notes as well...

Comment by Rob Reynolds [ 2016/08/24 ]

Verified as fixed. Now there is only one call that states Account currently disabled. It does not make calls to attempt to change the password. Puppet also does not report any change.

Comment by Rob Reynolds [ 2016/08/24 ]

Release notes have been added.

Generated at Mon Dec 16 04:53:18 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.