Skip to content

Commit 0fb5ec5

Browse files
committed
(PUP-10999) Fix logon account setting for Windows services
Before this commit, most checks were done in `validate` and `munge` of the `logonaccount` and `logonpassword` parameters. This proved to be too early in case of manifests where the value of these parameters depended on other actions (such as creating the user, changing its password or adding the 'Logon as a service' right in the same manifest where we are setting it as logonaccount for a service). This commit ensures said munging and validation to happen only when checking insyncness (munging) and when changes are required (validation).
1 parent 95130f8 commit 0fb5ec5

File tree

5 files changed

+292
-186
lines changed

5 files changed

+292
-186
lines changed

acceptance/tests/resource/service/windows.rb

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,27 @@ def service_manifest(name, params)
4545
}
4646

4747
new_user = "tempUser#{rand(999999).to_i}"
48+
fresh_user = "freshUser#{rand(999999).to_i}"
49+
50+
fresh_user_manifest = <<-MANIFEST
51+
user { '#{fresh_user}':
52+
ensure => present,
53+
password => 'freshUserPassword',
54+
roles => 'SeServiceLogonRight'
55+
}
56+
57+
service { '#{mock_service_nofail[:name]}':
58+
logonaccount => '#{fresh_user}',
59+
logonpassword => 'freshUserPassword',
60+
require => User['#{fresh_user}']
61+
}
62+
MANIFEST
4863

4964
teardown do
65+
delete_service(agent, mock_service_nofail[:name])
5066
delete_service(agent, mock_service_long_start_stop[:name])
5167
on(agent, puppet("resource user #{new_user} ensure=absent"))
68+
on(agent, puppet("resource user #{fresh_user} ensure=absent"))
5269
end
5370

5471
agents.each do |agent|
@@ -87,10 +104,22 @@ def service_manifest(name, params)
87104
assert_service_properties_on(agent, mock_service_nofail[:name], State: 'Running')
88105
end
89106

90-
step 'Verify that we can change logonaccount, for an already running service, using SID' do
107+
step 'Verify that we can change logonaccount, for an already running service, using user created in the same manifest' do
91108
assert_service_properties_on(agent, mock_service_nofail[:name], StartName: 'LocalSystem')
109+
apply_manifest_on(agent, fresh_user_manifest, expect_changes: true)
110+
assert_service_properties_on(agent, mock_service_nofail[:name], StartName: fresh_user)
111+
end
112+
113+
step 'Verify that running the same manifest twice causes no more changes' do
114+
assert_service_properties_on(agent, mock_service_nofail[:name], StartName: fresh_user)
115+
apply_manifest_on(agent, fresh_user_manifest, catch_changes: true)
116+
assert_service_properties_on(agent, mock_service_nofail[:name], StartName: fresh_user)
117+
end
118+
119+
step 'Verify that we can change logonaccount, for an already running service, using SID' do
120+
assert_service_properties_on(agent, mock_service_nofail[:name], StartName: fresh_user)
92121
on(agent, puppet("resource service #{mock_service_nofail[:name]} logonaccount=S-1-5-19")) do |result|
93-
assert_match(/Service\[#{mock_service_nofail[:name]}\]\/logonaccount: logonaccount changed 'LocalSystem' to '#{Regexp.escape(local_service_locale_name)}'/, result.stdout)
122+
assert_match(/Service\[#{mock_service_nofail[:name]}\]\/logonaccount: logonaccount changed '.\\#{fresh_user}' to '#{Regexp.escape(local_service_locale_name)}'/, result.stdout)
94123
assert_no_match(/Transitioning the #{mock_service_nofail[:name]} service from SERVICE_RUNNING to SERVICE_STOPPED/, result.stdout,
95124
"Expected no service restarts since ensure isn't being managed as 'running'.")
96125
assert_no_match(/Successfully started the #{mock_service_nofail[:name]} service/, result.stdout)
@@ -218,7 +247,7 @@ def service_manifest(name, params)
218247
end
219248

220249
step 'Verify that a user with `Logon As A Service` right denied will raise error when managing it as logonaccount for a service' do
221-
apply_manifest_on(agent, service_manifest(mock_service_nofail[:name], logonaccount: new_user), :acceptable_exit_codes => [1], catch_changes: true) do |result|
250+
apply_manifest_on(agent, service_manifest(mock_service_nofail[:name], logonaccount: new_user), :acceptable_exit_codes => [1, 4]) do |result|
222251
assert_match(/#{new_user}\" has the 'Log On As A Service' right set to denied./, result.stderr)
223252
end
224253
assert_service_properties_on(agent, mock_service_nofail[:name], StartName: new_user, State: 'Stopped')

lib/puppet/provider/service/windows.rb

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,17 +128,55 @@ def self.instances
128128
services
129129
end
130130

131+
def logonaccount_insync?(current)
132+
@normalized_logon_account ||= normalize_logonaccount
133+
@resource[:logonaccount] = @normalized_logon_account
134+
135+
insync = @resource[:logonaccount] == current
136+
self.logonpassword = @resource[:logonpassword] if insync
137+
insync
138+
end
139+
131140
def logonaccount
132141
return unless Puppet::Util::Windows::Service.exists?(@resource[:name])
133142
Puppet::Util::Windows::Service.logon_account(@resource[:name])
134143
end
135144

136145
def logonaccount=(value)
146+
validate_logon_credentials
137147
Puppet::Util::Windows::Service.set_startup_configuration(@resource[:name], options: {logon_account: value, logon_password: @resource[:logonpassword]})
138148
restart if @resource[:ensure] == :running && [:running, :paused].include?(status)
139149
end
140150

141151
def logonpassword=(value)
152+
validate_logon_credentials
142153
Puppet::Util::Windows::Service.set_startup_configuration(@resource[:name], options: {logon_password: value})
143154
end
155+
156+
private
157+
158+
def normalize_logonaccount
159+
logon_account = @resource[:logonaccount].sub(/^\.\\/, "#{Puppet::Util::Windows::ADSI.computer_name}\\")
160+
return 'LocalSystem' if Puppet::Util::Windows::User::localsystem?(logon_account)
161+
162+
@logonaccount_information ||= Puppet::Util::Windows::SID.name_to_principal(logon_account)
163+
return logon_account unless @logonaccount_information
164+
return ".\\#{@logonaccount_information.account}" if @logonaccount_information.domain == Puppet::Util::Windows::ADSI.computer_name
165+
@logonaccount_information.domain_account
166+
end
167+
168+
def validate_logon_credentials
169+
unless Puppet::Util::Windows::User::localsystem?(@normalized_logon_account)
170+
raise Puppet::Error.new("\"#{@normalized_logon_account}\" is not a valid account") unless @logonaccount_information && [:SidTypeUser, :SidTypeWellKnownGroup].include?(@logonaccount_information.account_type)
171+
172+
user_rights = Puppet::Util::Windows::User::get_rights(@logonaccount_information.domain_account) unless Puppet::Util::Windows::User::default_system_account?(@normalized_logon_account)
173+
raise Puppet::Error.new("\"#{@normalized_logon_account}\" has the 'Log On As A Service' right set to denied.") if user_rights =~ /SeDenyServiceLogonRight/
174+
raise Puppet::Error.new("\"#{@normalized_logon_account}\" is missing the 'Log On As A Service' right.") unless user_rights.nil? || user_rights =~ /SeServiceLogonRight/
175+
end
176+
177+
is_a_predefined_local_account = Puppet::Util::Windows::User::default_system_account?(@normalized_logon_account) || @normalized_logon_account == 'LocalSystem'
178+
account_info = @normalized_logon_account.split("\\")
179+
able_to_logon = Puppet::Util::Windows::User.password_is?(account_info[1], @resource[:logonpassword], account_info[0]) unless is_a_predefined_local_account
180+
raise Puppet::Error.new("The given password is invalid for user '#{@normalized_logon_account}'.") unless is_a_predefined_local_account || able_to_logon
181+
end
144182
end

lib/puppet/type/service.rb

Lines changed: 10 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -139,42 +139,17 @@ def sync
139139
newproperty(:logonaccount, :required_features => :manages_logon_credentials) do
140140
desc "Specify an account for service logon"
141141

142-
munge do |value|
143-
return value unless Puppet::Util::Platform.windows?
144-
return 'LocalSystem' if Puppet::Util::Windows::User::localsystem?(value)
145-
146-
value.sub!(/^\.\\/, "#{Puppet::Util::Windows::ADSI.computer_name}\\")
147-
user_information = Puppet::Util::Windows::SID.name_to_principal(value)
148-
raise Puppet::Error.new("\"#{value}\" is not a valid account") unless user_information && [:SidTypeUser, :SidTypeWellKnownGroup].include?(user_information.account_type)
149-
150-
user_rights = Puppet::Util::Windows::User::get_rights(user_information.domain_account) unless Puppet::Util::Windows::User::default_system_account?(value)
151-
raise Puppet::Error.new("\"#{user_information.domain_account}\" has the 'Log On As A Service' right set to denied.") if user_rights =~ /SeDenyServiceLogonRight/
152-
raise Puppet::Error.new("\"#{user_information.domain_account}\" is missing the 'Log On As A Service' right.") unless user_rights.nil? || user_rights =~ /SeServiceLogonRight/
153-
154-
if user_information.domain == Puppet::Util::Windows::ADSI.computer_name
155-
".\\#{user_information.account}"
156-
else
157-
user_information.domain_account
158-
end
142+
def insync?(current)
143+
return provider.logonaccount_insync?(current) if provider.respond_to?(:logonaccount_insync?)
144+
super(current)
159145
end
160146
end
161147

162148
newparam(:logonpassword, :required_features => :manages_logon_credentials) do
163149
desc "Specify a password for service logon. Default value is an empty string (when logonaccount is specified)."
164150

165151
validate do |value|
166-
raise Puppet::Error.new(_"The 'logonaccount' parameter is mandatory when setting 'logonpassword'.") unless @resource[:logonaccount]
167-
raise ArgumentError, _("Passwords cannot include ':'") if value.is_a?(String) and value.include?(":")
168-
return unless Puppet::Util::Platform.windows?
169-
170-
is_a_predefined_local_account = Puppet::Util::Windows::User::default_system_account?(@resource[:logonaccount]) || @resource[:logonaccount] == 'LocalSystem'
171-
172-
account_info = @resource[:logonaccount].split("\\")
173-
able_to_logon = Puppet::Util::Windows::User.password_is?(account_info[1], value, account_info[0]) unless is_a_predefined_local_account
174-
175-
raise Puppet::Error.new("The given password is invalid for user '#{@resource[:logonaccount]}'.") unless is_a_predefined_local_account || able_to_logon
176-
177-
provider.logonpassword=(value)
152+
raise ArgumentError, _("Passwords cannot include ':'") if value.is_a?(String) && value.include?(":")
178153
end
179154

180155
sensitive true
@@ -320,5 +295,11 @@ def refresh
320295
def self.needs_ensure_retrieved
321296
false
322297
end
298+
299+
validate do
300+
if @parameters[:logonpassword] && @parameters[:logonaccount].nil?
301+
raise Puppet::Error.new(_"The 'logonaccount' parameter is mandatory when setting 'logonpassword'.")
302+
end
303+
end
323304
end
324305
end

spec/unit/provider/service/windows_spec.rb

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,4 +271,206 @@
271271
}.to raise_error(Puppet::Error, /Cannot enable #{name}/)
272272
end
273273
end
274+
275+
describe "when managing logon credentials" do
276+
before do
277+
allow(Puppet::Util::Windows::ADSI).to receive(:computer_name).and_return(computer_name)
278+
allow(Puppet::Util::Windows::SID).to receive(:name_to_principal).and_return(principal)
279+
allow(Puppet::Util::Windows::Service).to receive(:set_startup_configuration).and_return(nil)
280+
end
281+
282+
let(:computer_name) { 'myPC' }
283+
284+
describe "#logonaccount=" do
285+
before do
286+
allow(Puppet::Util::Windows::User).to receive(:password_is?).and_return(true)
287+
resource[:logonaccount] = user_input
288+
provider.logonaccount_insync?(user_input)
289+
end
290+
291+
let(:user_input) { principal.account }
292+
let(:principal) do
293+
Puppet::Util::Windows::SID::Principal.new("myUser", nil, nil, computer_name, :SidTypeUser)
294+
end
295+
296+
context "when given user is 'myUser'" do
297+
it "should fail when the `Log On As A Service` right is missing from given user" do
298+
allow(Puppet::Util::Windows::User).to receive(:get_rights).with(principal.domain_account).and_return("")
299+
expect { provider.logonaccount=(user_input) }.to raise_error(Puppet::Error, /".\\#{principal.account}" is missing the 'Log On As A Service' right./)
300+
end
301+
302+
it "should fail when the `Log On As A Service` right is set to denied for given user" do
303+
allow(Puppet::Util::Windows::User).to receive(:get_rights).with(principal.domain_account).and_return("SeDenyServiceLogonRight")
304+
expect { provider.logonaccount=(user_input) }.to raise_error(Puppet::Error, /".\\#{principal.account}" has the 'Log On As A Service' right set to denied./)
305+
end
306+
307+
it "should not fail when given user has the `Log On As A Service` right" do
308+
allow(Puppet::Util::Windows::User).to receive(:get_rights).with(principal.domain_account).and_return("SeServiceLogonRight")
309+
expect { provider.logonaccount=(user_input) }.not_to raise_error
310+
end
311+
312+
['myUser', 'myPC\\myUser', ".\\myUser", "MYPC\\mYuseR"].each do |user_input_variant|
313+
let(:user_input) { user_input_variant }
314+
315+
it "should succesfully munge #{user_input_variant} to '.\\myUser'" do
316+
allow(Puppet::Util::Windows::User).to receive(:get_rights).with(principal.domain_account).and_return("SeServiceLogonRight")
317+
expect { provider.logonaccount=(user_input) }.not_to raise_error
318+
expect(resource[:logonaccount]).to eq(".\\myUser")
319+
end
320+
end
321+
end
322+
323+
context "when given user is a system account" do
324+
before do
325+
allow(Puppet::Util::Windows::User).to receive(:default_system_account?).and_return(true)
326+
end
327+
328+
let(:user_input) { principal.account }
329+
let(:principal) do
330+
Puppet::Util::Windows::SID::Principal.new("LOCAL SERVICE", nil, nil, "NT AUTHORITY", :SidTypeUser)
331+
end
332+
333+
it "should not fail when given user is a default system account even if the `Log On As A Service` right is missing" do
334+
expect(Puppet::Util::Windows::User).not_to receive(:get_rights)
335+
expect { provider.logonaccount=(user_input) }.not_to raise_error
336+
end
337+
338+
['LocalSystem', '.\LocalSystem', 'myPC\LocalSystem', 'lOcALsysTem'].each do |user_input_variant|
339+
let(:user_input) { user_input_variant }
340+
341+
it "should succesfully munge #{user_input_variant} to 'LocalSystem'" do
342+
expect { provider.logonaccount=(user_input) }.not_to raise_error
343+
expect(resource[:logonaccount]).to eq('LocalSystem')
344+
end
345+
end
346+
end
347+
348+
context "when domain is different from computer name" do
349+
before do
350+
allow(Puppet::Util::Windows::User).to receive(:get_rights).and_return("SeServiceLogonRight")
351+
end
352+
353+
context "when given user is from AD" do
354+
let(:user_input) { 'myRemoteUser' }
355+
let(:principal) do
356+
Puppet::Util::Windows::SID::Principal.new("myRemoteUser", nil, nil, "AD", :SidTypeUser)
357+
end
358+
359+
it "should not raise any error" do
360+
expect { provider.logonaccount=(user_input) }.not_to raise_error
361+
end
362+
363+
it "should succesfully be munged" do
364+
expect { provider.logonaccount=(user_input) }.not_to raise_error
365+
expect(resource[:logonaccount]).to eq('AD\myRemoteUser')
366+
end
367+
end
368+
369+
context "when given user is LocalService" do
370+
let(:user_input) { 'LocalService' }
371+
let(:principal) do
372+
Puppet::Util::Windows::SID::Principal.new("LOCAL SERVICE", nil, nil, "NT AUTHORITY", :SidTypeWellKnownGroup)
373+
end
374+
375+
it "should succesfully munge well known user" do
376+
expect { provider.logonaccount=(user_input) }.not_to raise_error
377+
expect(resource[:logonaccount]).to eq('NT AUTHORITY\LOCAL SERVICE')
378+
end
379+
end
380+
381+
context "when given user is in SID form" do
382+
let(:user_input) { 'S-1-5-20' }
383+
let(:principal) do
384+
Puppet::Util::Windows::SID::Principal.new("NETWORK SERVICE", nil, nil, "NT AUTHORITY", :SidTypeUser)
385+
end
386+
387+
it "should succesfully munge" do
388+
expect { provider.logonaccount=(user_input) }.not_to raise_error
389+
expect(resource[:logonaccount]).to eq('NT AUTHORITY\NETWORK SERVICE')
390+
end
391+
end
392+
393+
context "when given user is actually a group" do
394+
let(:principal) do
395+
Puppet::Util::Windows::SID::Principal.new("Administrators", nil, nil, "BUILTIN", :SidTypeAlias)
396+
end
397+
let(:user_input) { 'Administrators' }
398+
399+
it "should fail when sid type is not user or well known user" do
400+
expect { provider.logonaccount=(user_input) }.to raise_error(Puppet::Error, /"BUILTIN\\#{user_input}" is not a valid account/)
401+
end
402+
end
403+
end
404+
end
405+
406+
describe "#logonpassword=" do
407+
before do
408+
allow(Puppet::Util::Windows::User).to receive(:get_rights).and_return('SeServiceLogonRight')
409+
resource[:logonaccount] = account
410+
resource[:logonpassword] = user_input
411+
provider.logonaccount_insync?(account)
412+
end
413+
414+
let(:account) { 'LocalSystem' }
415+
416+
describe "when given logonaccount is a predefined_local_account" do
417+
let(:user_input) { 'pass' }
418+
let(:principal) { nil }
419+
420+
it "should pass validation when given account is 'LocalSystem'" do
421+
allow(Puppet::Util::Windows::User).to receive(:localsystem?).with('LocalSystem').and_return(true)
422+
allow(Puppet::Util::Windows::User).to receive(:default_system_account?).with('LocalSystem').and_return(true)
423+
424+
expect(Puppet::Util::Windows::User).not_to receive(:password_is?)
425+
expect { provider.logonpassword=(user_input) }.not_to raise_error
426+
end
427+
428+
['LOCAL SERVICE', 'NETWORK SERVICE', 'SYSTEM'].each do |predefined_local_account|
429+
describe "when given account is #{predefined_local_account}" do
430+
let(:account) { 'predefined_local_account' }
431+
let(:principal) do
432+
Puppet::Util::Windows::SID::Principal.new(account, nil, nil, "NT AUTHORITY", :SidTypeUser)
433+
end
434+
435+
it "should pass validation" do
436+
allow(Puppet::Util::Windows::User).to receive(:localsystem?).with(principal.account).and_return(false)
437+
allow(Puppet::Util::Windows::User).to receive(:localsystem?).with(principal.domain_account).and_return(false)
438+
expect(Puppet::Util::Windows::User).to receive(:default_system_account?).with(principal.domain_account).and_return(true).twice
439+
440+
expect(Puppet::Util::Windows::User).not_to receive(:password_is?)
441+
expect { provider.logonpassword=(user_input) }.not_to raise_error
442+
end
443+
end
444+
end
445+
end
446+
447+
describe "when given logonaccount is not a predefined local account" do
448+
before do
449+
allow(Puppet::Util::Windows::User).to receive(:localsystem?).with(".\\#{principal.account}").and_return(false)
450+
allow(Puppet::Util::Windows::User).to receive(:default_system_account?).with(".\\#{principal.account}").and_return(false)
451+
end
452+
453+
let(:account) { 'myUser' }
454+
let(:principal) do
455+
Puppet::Util::Windows::SID::Principal.new(account, nil, nil, computer_name, :SidTypeUser)
456+
end
457+
458+
describe "when password is proven correct" do
459+
let(:user_input) { 'myPass' }
460+
it "should pass validation" do
461+
allow(Puppet::Util::Windows::User).to receive(:password_is?).with('myUser', 'myPass', '.').and_return(true)
462+
expect { provider.logonpassword=(user_input) }.not_to raise_error
463+
end
464+
end
465+
466+
describe "when password is not proven correct" do
467+
let(:user_input) { 'myWrongPass' }
468+
it "should not pass validation" do
469+
allow(Puppet::Util::Windows::User).to receive(:password_is?).with('myUser', 'myWrongPass', '.').and_return(false)
470+
expect { provider.logonpassword=(user_input) }.to raise_error(Puppet::Error, /The given password is invalid for user '.\\myUser'/)
471+
end
472+
end
473+
end
474+
end
475+
end
274476
end

0 commit comments

Comments
 (0)