From f37a44430f60f0e8f030bcfcff7677a61723995f Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Wed, 4 Apr 2018 11:28:10 +1000 Subject: [PATCH] win_service: fix when dealing with paths with special chars and change WMI to CIM cmdlets (#37897) * win_service: fix when dealing with paths with special chars and change WMI to CIM cmdlets * compare username in lowercase for test --- lib/ansible/modules/windows/win_service.ps1 | 130 +++++++++++------- lib/ansible/modules/windows/win_service.py | 4 +- .../targets/win_service/defaults/main.yml | 4 +- .../targets/win_service/tasks/tests.yml | 44 ++---- test/sanity/pslint/ignore.txt | 1 - 5 files changed, 97 insertions(+), 86 deletions(-) diff --git a/lib/ansible/modules/windows/win_service.ps1 b/lib/ansible/modules/windows/win_service.ps1 index 97aa44e3d90..cdd1232d0a6 100644 --- a/lib/ansible/modules/windows/win_service.ps1 +++ b/lib/ansible/modules/windows/win_service.ps1 @@ -1,23 +1,11 @@ #!powershell # This file is part of Ansible -# + # Copyright 2014, Chris Hoffman -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . - -# WANT_JSON -# POWERSHELL_COMMON +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +#Requires -Module Ansible.ModuleUtils.Legacy +#Requires -Module Ansible.ModuleUtils.SID $ErrorActionPreference = "Stop" @@ -39,11 +27,28 @@ $username = Get-AnsibleParam -obj $params -name 'username' -type 'str' $result = @{ changed = $false - warnings = @() } -if ($username -ne $null -and $password -eq $null) { - Fail-Json $result "The argument 'password' must be supplied with 'username'" +# parse the username to SID and back so we get the full username with domain in a way WMI understands +if ($username -ne $null) { + if ($username -eq "LocalSystem") { + $username_sid = "S-1-5-18" + } else { + $username_sid = Convert-ToSID -account_name $username + } + + # the SYSTEM account is a special beast, Win32_Service Change requires StartName to be LocalSystem + # to specify LocalSystem/NT AUTHORITY\SYSTEM + if ($username_sid -eq "S-1-5-18") { + $username = "LocalSystem" + $password = $null + } else { + # Win32_Service, password must be "" and not $null when setting to LocalService or NetworkService + if ($username_sid -in @("S-1-5-19", "S-1-5-20")) { + $password = "" + } + $username = Convert-FromSID -sid $username_sid + } } if ($password -ne $null -and $username -eq $null) { Fail-Json $result "The argument 'username' must be supplied with 'password'" @@ -57,8 +62,8 @@ if ($path -ne $null) { Function Get-ServiceInfo($name) { # Need to get new objects so we have the latest info - $svc = Get-Service -Name $name - $wmi_svc = Get-WmiObject Win32_Service | Where-Object { $_.Name -eq $svc.Name } + $svc = Get-Service | Where-Object { $_.Name -eq $name -or $_.DisplayName -eq $name } + $wmi_svc = Get-CimInstance -ClassName Win32_Service -Filter "name='$($svc.Name)'" # Delayed start_mode is in reality Automatic (Delayed), need to check reg key for type $delayed = Get-DelayedStatus -name $svc.Name @@ -91,8 +96,8 @@ Function Get-ServiceInfo($name) { $result.start_mode = $actual_start_mode $result.path = $wmi_svc.PathName $result.description = $description - $result.username = $wmi_svc.startname - $result.desktop_interact = (ConvertTo-Bool $wmi_svc.DesktopInteract) + $result.username = $wmi_svc.StartName + $result.desktop_interact = $wmi_svc.DesktopInteract $result.dependencies = $existing_dependencies $result.depended_by = $existing_depended_by $result.can_pause_and_continue = $svc.CanPauseAndContinue @@ -132,7 +137,7 @@ Function Get-WmiErrorMessage($return_value) { Function Get-DelayedStatus($name) { $delayed_key = "HKLM:\System\CurrentControlSet\Services\$name" try { - $delayed = ConvertTo-Bool ((Get-ItemProperty -Path $delayed_key).DelayedAutostart) + $delayed = ConvertTo-Bool ((Get-ItemProperty -LiteralPath $delayed_key).DelayedAutostart) } catch { $delayed = $false } @@ -146,14 +151,14 @@ Function Set-ServiceStartMode($svc, $start_mode) { $delayed_key = "HKLM:\System\CurrentControlSet\Services\$($svc.Name)" # Original start up type was auto (delayed) and we want auto, need to removed delayed key if ($start_mode -eq 'auto' -and $result.start_mode -eq 'delayed') { - Set-ItemProperty -Path $delayed_key -Name "DelayedAutostart" -Value 0 -Type DWORD -WhatIf:$check_mode + Set-ItemProperty -LiteralPath $delayed_key -Name "DelayedAutostart" -Value 0 -WhatIf:$check_mode # Original start up type was auto and we want auto (delayed), need to add delayed key } elseif ($start_mode -eq 'delayed' -and $result.start_mode -eq 'auto') { - Set-ItemProperty -Path $delayed_key -Name "DelayedAutostart" -Value 1 -Type DWORD -WhatIf:$check_mode + Set-ItemProperty -LiteralPath $delayed_key -Name "DelayedAutostart" -Value 1 -WhatIf:$check_mode # Original start up type was not auto or auto (delayed), need to change to auto and add delayed key } elseif ($start_mode -eq 'delayed') { $svc | Set-Service -StartupType "auto" -WhatIf:$check_mode - Set-ItemProperty -Path $delayed_key -Name "DelayedAutostart" -Value 1 -Type DWORD -WhatIf:$check_mode + Set-ItemProperty -LiteralPath $delayed_key -Name "DelayedAutostart" -Value 1 -WhatIf:$check_mode # Original start up type was not what we were looking for, just change to that type } else { $svc | Set-Service -StartupType $start_mode -WhatIf:$check_mode @@ -166,13 +171,30 @@ Function Set-ServiceStartMode($svc, $start_mode) { } } -Function Set-ServiceAccount($wmi_svc, $username, $password) { - if ($result.username -ne $username) { +Function Set-ServiceAccount($wmi_svc, $username_sid, $username, $password) { + if ($result.username -eq "LocalSystem") { + $actual_sid = "S-1-5-18" + } else { + $actual_sid = Convert-ToSID -account_name $result.username + } + + if ($actual_sid -ne $username_sid) { + $change_arguments = @{ + StartName = $username + StartPassword = $password + DesktopInteract = $result.desktop_interact + } + # need to disable desktop interact when not using the SYSTEM account + if ($username_sid -ne "S-1-5-18") { + $change_arguments.DesktopInteract = $false + } + #WMI.Change doesn't support -WhatIf, cannot fully test with check_mode if (-not $check_mode) { - $return = $wmi_svc.Change($null,$null,$null,$null,$null,$false,$username,$password,$null,$null,$null) + $return = $wmi_svc | Invoke-CimMethod -MethodName Change -Arguments $change_arguments if ($return.ReturnValue -ne 0) { - Fail-Json $result "$($return.ReturnValue): $(Get-WmiErrorMessage -return_value $return.ReturnValue)" + $error_msg = Get-WmiErrorMessage -return_value $result.ReturnValue + Fail-Json -obj $result -message "Failed to set service account to $($username): $($return.ReturnValue) - $error_msg" } } @@ -183,9 +205,10 @@ Function Set-ServiceAccount($wmi_svc, $username, $password) { Function Set-ServiceDesktopInteract($wmi_svc, $desktop_interact) { if ($result.desktop_interact -ne $desktop_interact) { if (-not $check_mode) { - $return = $wmi_svc.Change($null,$null,$null,$null,$null,$desktop_interact,$null,$null,$null,$null,$null) + $return = $wmi_svc | Invoke-CimMethod -MethodName Change -Arguments @{DesktopInteract = $desktop_interact} if ($return.ReturnValue -ne 0) { - Fail-Json $result "$($return.ReturnValue): $(Get-WmiErrorMessage -return_value $return.ReturnValue)" + $error_msg = Get-WmiErrorMessage -return_value $return.ReturnValue + Fail-Json -obj $result -message "Failed to set desktop interact $($desktop_interact): $($return.ReturnValue) - $error_msg" } } @@ -220,7 +243,7 @@ Function Set-ServiceDescription($svc, $description) { Function Set-ServicePath($name, $path) { if ($result.path -ne $path) { try { - Set-ItemProperty -Path "HKLM:\System\CurrentControlSet\Services\$name" -Name ImagePath -Value $path -WhatIf:$check_mode + Set-ItemProperty -LiteralPath "HKLM:\System\CurrentControlSet\Services\$name" -Name ImagePath -Value $path -WhatIf:$check_mode } catch { Fail-Json $result $_.Exception.Message } @@ -266,9 +289,11 @@ Function Set-ServiceDependencies($wmi_svc, $dependency_action, $dependencies) { if ($will_change -eq $true) { if (-not $check_mode) { - $return = $wmi_svc.Change($null,$null,$null,$null,$null,$null,$null,$null,$null,$null,$new_dependencies) + $return = $wmi_svc | Invoke-CimMethod -MethodName Change -Arguments @{ServiceDependencies = $new_dependencies} if ($return.ReturnValue -ne 0) { - Fail-Json $result "$($return.ReturnValue): $(Get-WmiErrorMessage -return_value $return.ReturnValue)" + $error_msg = Get-WmiErrorMessage -return_value $return.ReturnValue + $dep_string = $new_dependencies -join ", " + Fail-Json -obj $result -message "Failed to set service dependencies $($dep_string): $($return.ReturnValue) - $error_msg" } } @@ -280,13 +305,13 @@ Function Set-ServiceState($svc, $wmi_svc, $state) { if ($state -eq "started" -and $result.state -ne "running") { if ($result.state -eq "paused") { try { - Resume-Service -Name $svc.Name -WhatIf:$check_mode + $svc | Resume-Service -WhatIf:$check_mode } catch { Fail-Json $result "failed to start service from paused state $($svc.Name): $($_.Exception.Message)" } } else { try { - Start-Service -Name $svc.Name -WhatIf:$check_mode + $svc | Start-Service -WhatIf:$check_mode } catch { Fail-Json $result $_.Exception.Message } @@ -297,7 +322,7 @@ Function Set-ServiceState($svc, $wmi_svc, $state) { if ($state -eq "stopped" -and $result.state -ne "stopped") { try { - Stop-Service -Name $svc.Name -Force:$force_dependent_services -WhatIf:$check_mode + $svc | Stop-Service -Force:$force_dependent_services -WhatIf:$check_mode } catch { Fail-Json $result $_.Exception.Message } @@ -307,7 +332,7 @@ Function Set-ServiceState($svc, $wmi_svc, $state) { if ($state -eq "restarted") { try { - Restart-Service -Name $svc.Name -Force:$force_dependent_services -WhatIf:$check_mode + $svc | Restart-Service -Force:$force_dependent_services -WhatIf:$check_mode } catch { Fail-Json $result $_.Exception.Message } @@ -322,7 +347,7 @@ Function Set-ServiceState($svc, $wmi_svc, $state) { } try { - Suspend-Service -Name $svc.Name -WhatIf:$check_mode + $svc | Suspend-Service -WhatIf:$check_mode } catch { Fail-Json $result "failed to pause service $($svc.Name): $($_.Exception.Message)" } @@ -331,14 +356,15 @@ Function Set-ServiceState($svc, $wmi_svc, $state) { if ($state -eq "absent") { try { - Stop-Service -Name $svc.Name -Force:$force_dependent_services -WhatIf:$check_mode + $svc | Stop-Service -Force:$force_dependent_services -WhatIf:$check_mode } catch { Fail-Json $result $_.Exception.Message } if (-not $check_mode) { - $return = $wmi_svc.Delete() + $return = $wmi_svc | Invoke-CimMethod -MethodName Delete if ($return.ReturnValue -ne 0) { - Fail-Json $result "$($return.ReturnValue): $(Get-WmiErrorMessage -return_value $return.ReturnValue)" + $error_msg = Get-WmiErrorMessage -return_value $return.ReturnValue + Fail-Json -obj $result -message "Failed to delete service $($svc.Name): $($return.ReturnValue) - $error_msg" } } @@ -347,7 +373,7 @@ Function Set-ServiceState($svc, $wmi_svc, $state) { } Function Set-ServiceConfiguration($svc) { - $wmi_svc = Get-WmiObject Win32_Service | Where-Object { $_.Name -eq $svc.Name } + $wmi_svc = Get-CimInstance -ClassName Win32_Service -Filter "name='$($svc.Name)'" Get-ServiceInfo -name $svc.Name if ($desktop_interact -eq $true -and (-not ($result.username -eq 'LocalSystem' -or $username -eq 'LocalSystem'))) { Fail-Json $result "Can only set desktop_interact to true when service is run with/or 'username' equals 'LocalSystem'" @@ -358,7 +384,7 @@ Function Set-ServiceConfiguration($svc) { } if ($username -ne $null) { - Set-ServiceAccount -wmi_svc $wmi_svc -username $username -password $password + Set-ServiceAccount -wmi_svc $wmi_svc -username_sid $username_sid -username $username -password $password } if ($display_name -ne $null) { @@ -386,7 +412,9 @@ Function Set-ServiceConfiguration($svc) { } } -$svc = Get-Service -Name $name -ErrorAction SilentlyContinue +# need to use Where-Object as -Name doesn't work with [] in the service name +# https://github.com/ansible/ansible/issues/37621 +$svc = Get-Service | Where-Object { $_.Name -eq $name -or $_.DisplayName -eq $name } if ($svc) { Set-ServiceConfiguration -svc $svc } else { @@ -401,7 +429,7 @@ if ($svc) { } $result.changed = $true - $svc = Get-Service -Name $name + $svc = Get-Service | Where-Object { $_.Name -eq $name } Set-ServiceConfiguration -svc $svc } else { # We will only reach here if the service is installed and the state is not absent @@ -425,14 +453,12 @@ if ($svc) { if ($state -eq 'absent') { # Recreate result so it doesn't have the extra meta data now that is has been deleted $changed = $result.changed - $warnings = $result.warnings $result = @{ changed = $changed - warnings = $warnings exists = $false } } elseif ($svc -ne $null) { Get-ServiceInfo -name $name } -Exit-Json $result +Exit-Json -obj $result diff --git a/lib/ansible/modules/windows/win_service.py b/lib/ansible/modules/windows/win_service.py index c6ba56471d1..3110348c654 100644 --- a/lib/ansible/modules/windows/win_service.py +++ b/lib/ansible/modules/windows/win_service.py @@ -95,7 +95,9 @@ options: username: description: - The username to set the service to start as. - - This and the C(password) argument must be supplied together. + - This and the C(password) argument must be supplied together when using + a local or domain account. + - Set to C(LocalSystem) to use the SYSTEM account. version_added: "2.3" notes: - For non-Windows targets, use the M(service) module instead. diff --git a/test/integration/targets/win_service/defaults/main.yml b/test/integration/targets/win_service/defaults/main.yml index 74685a6c50a..828eda2817e 100644 --- a/test/integration/targets/win_service/defaults/main.yml +++ b/test/integration/targets/win_service/defaults/main.yml @@ -1,9 +1,9 @@ --- # parameters set here for creating new service in tests -test_win_service_name: TestService +test_win_service_name: TestService [*abc] test_win_service_display_name: Test Service test_win_service_description: Test Service description test_win_service_path: C:\Windows\System32\snmptrap.exe # used for the pause tests, need to use an actual service -test_win_service_pause_name: TapiSrv \ No newline at end of file +test_win_service_pause_name: TapiSrv diff --git a/test/integration/targets/win_service/tasks/tests.yml b/test/integration/targets/win_service/tasks/tests.yml index 8d4c80a124b..da53dc37ff2 100644 --- a/test/integration/targets/win_service/tasks/tests.yml +++ b/test/integration/targets/win_service/tasks/tests.yml @@ -314,13 +314,6 @@ - win_service_stopped_again is not changed - win_service_stopped_again.state == 'stopped' -- name: set username without password - win_service: - name: "{{test_win_service_name}}" - username: username - register: win_service_change_user_without_password - failed_when: win_service_change_user_without_password.msg != "The argument 'password' must be supplied with 'username'" - - name: set password without username win_service: name: "{{test_win_service_name}}" @@ -332,7 +325,6 @@ win_service: name: "{{test_win_service_name}}" username: NT AUTHORITY\NetworkService - password: "" desktop_interact: True register: win_desktop_interact_not_local_system failed_when: win_desktop_interact_not_local_system.msg != "Can only set 'desktop_interact' to true when 'username' equals 'LocalSystem'" @@ -341,28 +333,26 @@ win_service: name: "{{test_win_service_name}}" username: NT AUTHORITY\NetworkService - password: "" register: win_service_change_password_network_service - name: check that the service user has been set to Network Service assert: that: - win_service_change_password_network_service is changed - - win_service_change_password_network_service.username == 'NT AUTHORITY\\NetworkService' + - win_service_change_password_network_service.username == 'NT AUTHORITY\\NETWORK SERVICE' - win_service_change_password_network_service.desktop_interact == False - name: set service username to Network Service again win_service: name: "{{test_win_service_name}}" username: NT AUTHORITY\NetworkService - password: "" register: win_service_change_password_network_service_again - name: check that the service user has been set to Network Service and nothing changed assert: that: - win_service_change_password_network_service_again is not changed - - win_service_change_password_network_service_again.username == 'NT AUTHORITY\\NetworkService' + - win_service_change_password_network_service_again.username == 'NT AUTHORITY\\NETWORK SERVICE' - win_service_change_password_network_service_again.desktop_interact == False - name: set service username to interact with desktop with existing user to fail @@ -376,35 +366,32 @@ win_service: name: "{{test_win_service_name}}" username: NT AUTHORITY\LocalService - password: "" register: win_service_change_password_local_service - name: check that the service user has been set to Local Service assert: that: - win_service_change_password_local_service is changed - - win_service_change_password_local_service.username == 'NT AUTHORITY\\LocalService' + - win_service_change_password_local_service.username == 'NT AUTHORITY\\LOCAL SERVICE' - win_service_change_password_local_service.desktop_interact == False - name: set service username to Local Service again win_service: name: "{{test_win_service_name}}" username: NT AUTHORITY\LocalService - password: "" register: win_service_change_password_local_service_again - name: check that the service user has been set to Local Service and nothing changed assert: that: - win_service_change_password_local_service_again is not changed - - win_service_change_password_local_service_again.username == 'NT AUTHORITY\\LocalService' + - win_service_change_password_local_service_again.username == 'NT AUTHORITY\\LOCAL SERVICE' - win_service_change_password_local_service_again.desktop_interact == False - name: set service username to Local System win_service: name: "{{test_win_service_name}}" username: LocalSystem - password: "" register: win_service_change_password_local_system - name: check that the service user has been set to Local System @@ -418,7 +405,6 @@ win_service: name: "{{test_win_service_name}}" username: LocalSystem - password: "" register: win_service_change_password_local_system_again - name: check that the service user has been set to Local System and nothing changed @@ -431,8 +417,7 @@ - name: set service username to Local System with desktop interaction win_service: name: "{{test_win_service_name}}" - username: LocalSystem - password: "" + username: SYSTEM # tests that you can also set it this way desktop_interact: True register: win_service_local_system_desktop @@ -446,8 +431,7 @@ - name: set service username to Local System with desktop interaction again win_service: name: "{{test_win_service_name}}" - username: LocalSystem - password: "" + username: SYSTEM desktop_interact: True register: win_service_local_system_desktop_again @@ -513,7 +497,7 @@ - name: set service username to current user win_service: name: "{{test_win_service_name}}" - username: ".\\{{ansible_user}}" + username: "{{ansible_user}}" password: "{{ansible_password}}" register: win_service_change_password_current_user @@ -521,13 +505,13 @@ assert: that: - win_service_change_password_current_user is changed - - win_service_change_password_current_user.username == '.\\{{ansible_user}}' + - win_service_change_password_current_user.username|lower == '.\\{{ansible_user|lower}}' - win_service_change_password_current_user.desktop_interact == False - name: set service username to current user again win_service: name: "{{test_win_service_name}}" - username: ".\\{{ansible_user}}" + username: "{{ansible_user}}" password: "{{ansible_password}}" register: win_service_change_password_current_user_again @@ -535,7 +519,7 @@ assert: that: - win_service_change_password_current_user_again is not changed - - win_service_change_password_current_user_again.username == '.\\{{ansible_user}}' + - win_service_change_password_current_user_again.username|lower == '.\\{{ansible_user|lower}}' - win_service_change_password_current_user_again.desktop_interact == False - name: set service display name @@ -689,7 +673,7 @@ assert: that: - win_service_dependency_add is changed - - win_service_dependency_add.dependencies == ['{{test_win_service_name}}', 'TestServiceParent2'] + - win_service_dependency_add.dependencies == ['TestServiceParent2', '{{test_win_service_name}}'] - name: add another dependency to service again win_service: @@ -702,7 +686,7 @@ assert: that: - win_service_dependency_add_again is not changed - - win_service_dependency_add_again.dependencies == ['{{test_win_service_name}}', 'TestServiceParent2'] + - win_service_dependency_add_again.dependencies == ['TestServiceParent2', '{{test_win_service_name}}'] - name: remove another dependency to service win_service: @@ -741,7 +725,7 @@ assert: that: - win_service_dependency_set_list is changed - - win_service_dependency_set_list.dependencies == ['{{test_win_service_name}}', 'TestServiceParent2'] + - win_service_dependency_set_list.dependencies == ['TestServiceParent2', '{{test_win_service_name}}'] - name: make sure all services are stopped, set to LocalSystem and set to auto start before next test win_service: @@ -794,7 +778,7 @@ name: "{{test_win_service_name}}" state: absent register: win_service_removed_failed - failed_when: win_service_removed_failed.msg != "Cannot stop service 'Test Service New (TestService)' because it has dependent services. It can only be stopped if the Force flag is set." + failed_when: win_service_removed_failed.msg != "Cannot stop service 'Test Service New (" + test_win_service_name + ")' because it has dependent services. It can only be stopped if the Force flag is set." - name: remove the service while ignoring dependencies win_service: diff --git a/test/sanity/pslint/ignore.txt b/test/sanity/pslint/ignore.txt index dc4d278cf63..42169167d0e 100644 --- a/test/sanity/pslint/ignore.txt +++ b/test/sanity/pslint/ignore.txt @@ -100,7 +100,6 @@ lib/ansible/modules/windows/win_security_policy.ps1 PSUseApprovedVerbs lib/ansible/modules/windows/win_security_policy.ps1 PSUseDeclaredVarsMoreThanAssignments lib/ansible/modules/windows/win_service.ps1 PSAvoidUsingPlainTextForPassword lib/ansible/modules/windows/win_service.ps1 PSAvoidUsingUserNameAndPassWordParams -lib/ansible/modules/windows/win_service.ps1 PSAvoidUsingWMICmdlet lib/ansible/modules/windows/win_shell.ps1 PSAvoidUsingCmdletAliases lib/ansible/modules/windows/win_shell.ps1 PSUseApprovedVerbs lib/ansible/modules/windows/win_stat.ps1 PSAvoidUsingWMICmdlet