Skip to content

Commit c7072d5

Browse files
Merge pull request #8566 from luchihoratiu/PUP-10999
(PUP-10999) Fix logon account setting for Windows services
2 parents 409536e + 0fb5ec5 commit c7072d5

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)