From c85f28481a7ee6308e4dc3e29ad26f05c38c9432 Mon Sep 17 00:00:00 2001 From: Stuart Preston Date: Wed, 25 Jul 2018 10:04:17 +0100 Subject: Assume credentials supplied are still valid if they cannot be validated due to a Windows account restriction (#7416) Signed-off-by: Stuart Preston --- lib/chef/util/windows/net_user.rb | 12 ++- spec/functional/resource/user/windows_spec.rb | 144 +++++++++++++++++++++++--- spec/unit/win32/error_spec.rb | 14 ++- 3 files changed, 149 insertions(+), 21 deletions(-) diff --git a/lib/chef/util/windows/net_user.rb b/lib/chef/util/windows/net_user.rb index 9231afe46f..5d0dbf7a53 100644 --- a/lib/chef/util/windows/net_user.rb +++ b/lib/chef/util/windows/net_user.rb @@ -29,6 +29,7 @@ class Chef::Util::Windows::NetUser < Chef::Util::Windows NetUser = Chef::ReservedNames::Win32::NetUser Security = Chef::ReservedNames::Win32::Security + Win32APIError = Chef::ReservedNames::Win32::API::Error USER_INFO_3_TRANSFORM = { name: :usri3_name, @@ -96,8 +97,15 @@ class Chef::Util::Windows::NetUser < Chef::Util::Windows token = Security.logon_user(@username, nil, passwd, LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT) true - rescue Chef::Exceptions::Win32APIError - false + rescue Chef::Exceptions::Win32APIError => e + Chef::Log.trace(e) + # we're only interested in the incorrect password failures + if e.to_s =~ /System Error Code: 1326/ + return false + end + # all other exceptions will assume we cannot logon for a different reason + Chef::Log.trace("Unable to login with the specified credentials. Assuming the credentials are valid.") + true end def get_info diff --git a/spec/functional/resource/user/windows_spec.rb b/spec/functional/resource/user/windows_spec.rb index 56ae962ee4..c5792b1ea9 100644 --- a/spec/functional/resource/user/windows_spec.rb +++ b/spec/functional/resource/user/windows_spec.rb @@ -1,5 +1,7 @@ # Author:: Jay Mundrawala () -# Copyright:: Copyright 2015-2016, Chef Software +# Author:: Stuart Preston () +# +# Copyright:: Copyright 2015-2018, Chef Software # License:: Apache License, Version 2.0 # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -22,7 +24,7 @@ describe Chef::Provider::User::Windows, :windows_only do include Chef::Mixin::ShellOut let(:username) { "ChefFunctionalTest" } - let(:password) { SecureRandom.uuid } + let(:password) { "DummyP2ssw0rd!" } let(:node) do n = Chef::Node.new @@ -44,29 +46,137 @@ describe Chef::Provider::User::Windows, :windows_only do shell_out("net user #{u} /delete") end - before do + def backup_secedit_policy + backup_command = "secedit /export /cfg #{ENV['TEMP']}\\secedit_restore.inf /areas SECURITYPOLICY" + shell_out(backup_command) + end + + def restore_secedit_policy + security_database = "C:\\windows\\security\\database\\seceditnew.sdb" + restore_command = "secedit /configure /db #{security_database} /cfg #{ENV['TEMP']}\\secedit_restore.inf /areas SECURITYPOLICY" + shell_out(restore_command) + end + + def set_windows_minimum_password_length(minimum_password_length = 0) + require "tempfile" + temp_security_database = "C:\\windows\\security\\database\\seceditnew.sdb" + temp_security_template = Tempfile.new(["chefpolicy", ".inf"]) + file_content = <<~EOF + [Unicode] + Unicode=yes + [System Access] + MinimumPasswordLength = #{minimum_password_length} + PasswordComplexity = 0 + [Version] + signature="$CHICAGO$" + Revision=1 + EOF + windows_template_path = temp_security_template.path.gsub("/") { "\\" } + security_command = "secedit /configure /db #{temp_security_database} /cfg #{windows_template_path} /areas SECURITYPOLICY" + temp_security_template.write(file_content) + temp_security_template.close + shell_out(security_command) + end + + before(:all) do + backup_secedit_policy + end + + before(:each) do delete_user(username) allow(run_context).to receive(:logger).and_return(logger) end + after(:all) do + restore_secedit_policy + end + describe "action :create" do - it "creates a user when a username and password are given" do - new_resource.run_action(:create) - expect(new_resource).to be_updated_by_last_action - expect(shell_out("net user #{username}").exitstatus).to eq(0) - end + context "on a Windows system with a policy that requires non-blank passwords and no complexity requirements" do - it "reports no changes if there are no changes needed" do - new_resource.run_action(:create) - new_resource.run_action(:create) - expect(new_resource).not_to be_updated_by_last_action + before(:all) do + set_windows_minimum_password_length(1) + end + + context "when a username and non-empty password are given" do + it "creates a user" do + new_resource.run_action(:create) + expect(new_resource).to be_updated_by_last_action + expect(shell_out("net user #{username}").exitstatus).to eq(0) + end + + it "is idempotent" do + new_resource.run_action(:create) + new_resource.run_action(:create) + expect(new_resource).not_to be_updated_by_last_action + end + + it "allows changing the password" do + new_resource.run_action(:create) + new_resource.password(SecureRandom.uuid) + new_resource.run_action(:create) + expect(new_resource).to be_updated_by_last_action + end + end + + context "when a username and empty password are given" do + it "does not create the specified user" do + new_resource.password("") + expect { new_resource.run_action(:create) }.to raise_exception(Chef::Exceptions::Win32APIError, /The password does not meet the password policy requirements/) + end + end end - it "allows chaning the password" do - new_resource.run_action(:create) - new_resource.password(SecureRandom.uuid) - new_resource.run_action(:create) - expect(new_resource).to be_updated_by_last_action + context "on a Windows system with a policy that allows blank passwords" do + + before(:all) do + set_windows_minimum_password_length(0) + end + + context "when a username and non-empty password are given" do + it "creates a user" do + new_resource.run_action(:create) + expect(new_resource).to be_updated_by_last_action + expect(shell_out("net user #{username}").exitstatus).to eq(0) + end + + it "is idempotent" do + new_resource.run_action(:create) + new_resource.run_action(:create) + expect(new_resource).not_to be_updated_by_last_action + end + + it "allows changing the password" do + new_resource.run_action(:create) + new_resource.password(SecureRandom.uuid) + new_resource.run_action(:create) + expect(new_resource).to be_updated_by_last_action + end + end + + context "when a username and empty password are given" do + it "creates a user" do + new_resource.password("") + new_resource.run_action(:create) + expect(new_resource).to be_updated_by_last_action + expect(shell_out("net user #{username}").exitstatus).to eq(0) + end + + it "is idempotent" do + new_resource.password("") + new_resource.run_action(:create) + new_resource.run_action(:create) + expect(new_resource).not_to be_updated_by_last_action + end + + it "allows changing the password from empty to a value" do + new_resource.password("") + new_resource.run_action(:create) + new_resource.password(SecureRandom.uuid) + new_resource.run_action(:create) + expect(new_resource).to be_updated_by_last_action + end + end end context "with a gid specified" do diff --git a/spec/unit/win32/error_spec.rb b/spec/unit/win32/error_spec.rb index e8b6d5af6a..316303ab4f 100644 --- a/spec/unit/win32/error_spec.rb +++ b/spec/unit/win32/error_spec.rb @@ -36,12 +36,12 @@ describe "Chef::Win32::Error", :windows_only do end context "last error received is not user_not_found" do - it "raises Win32APIError exception" do + it "raises Win32APIError exception with code 2202" do expect(Chef::ReservedNames::Win32::Error).to receive(:get_last_error).and_return( Chef::ReservedNames::Win32::API::Error::ERROR_BAD_USERNAME ) expect(Chef::ReservedNames::Win32::Error).to receive_message_chain(:format_message, :strip).and_return("Bad Username") - expect { Chef::ReservedNames::Win32::Error.raise! }.to raise_error Chef::Exceptions::Win32APIError + expect { Chef::ReservedNames::Win32::Error.raise! }.to raise_error(Chef::Exceptions::Win32APIError) end end end @@ -62,6 +62,16 @@ describe "Chef::Win32::Error", :windows_only do expect { Chef::ReservedNames::Win32::Error.raise! nil, Chef::ReservedNames::Win32::API::Error::ERROR_BAD_USERNAME }.to raise_error Chef::Exceptions::Win32APIError end end + + context "last error received is logon failure" do + it "raises a Win32ApiError exception with code 1326" do + expect { Chef::ReservedNames::Win32::Error.raise! nil, Chef::ReservedNames::Win32::API::Error::ERROR_LOGON_FAILURE }.to raise_error { |error| + expect(error).to be_a(Chef::Exceptions::Win32APIError) + # This is not localized but neither is the exception. + expect(error.to_s).to match(/System Error Code: 1326/) + } + end + end end end end -- cgit v1.2.1