From be26f4916f6bfa07be119b91aadb5fa209f0bf13 Mon Sep 17 00:00:00 2001 From: Brian Scholer <1260690+briantist@users.noreply.github.com> Date: Mon, 17 Feb 2020 00:35:54 -0500 Subject: [PATCH] win_dns_client DHCP improvements + more (#66451) * Fix DHCP support in win_dns_client + more * Fix bugs and test failures, add changelog fragment * Add idempotency tests for DHCP * Address review feedback; dedup address-family code * Remove legacy function * Remove old reference --- .../66451-win_dns_client-dhcp-support.yml | 6 + .../modules/windows/win_dns_client.ps1 | 203 +++++++++++++----- lib/ansible/modules/windows/win_dns_client.py | 12 +- .../targets/win_dns_client/tasks/main.yml | 11 + 4 files changed, 178 insertions(+), 54 deletions(-) create mode 100644 changelogs/fragments/66451-win_dns_client-dhcp-support.yml diff --git a/changelogs/fragments/66451-win_dns_client-dhcp-support.yml b/changelogs/fragments/66451-win_dns_client-dhcp-support.yml new file mode 100644 index 00000000000..caddc30f583 --- /dev/null +++ b/changelogs/fragments/66451-win_dns_client-dhcp-support.yml @@ -0,0 +1,6 @@ +minor_changes: + - Checks for and resolves a condition where effective nameservers are obfucated, usually by malware. + See https://www.welivesecurity.com/2016/06/02/crouching-tiger-hidden-dns/ +bugfixes: + - Fix detection of DHCP setting so that resetting to DHCP doesn't cause ``CHANGED`` status on every run. + See https://github.com/ansible/ansible/issues/66450 diff --git a/lib/ansible/modules/windows/win_dns_client.ps1 b/lib/ansible/modules/windows/win_dns_client.ps1 index 2ca5294b18c..47eeb26b621 100644 --- a/lib/ansible/modules/windows/win_dns_client.ps1 +++ b/lib/ansible/modules/windows/win_dns_client.ps1 @@ -4,18 +4,16 @@ #Requires -Module Ansible.ModuleUtils.Legacy -# FUTURE: check statically-set values via registry so we can determine difference between DHCP-source values and static values? (prevent spurious changed -# notifications on DHCP-sourced values) - Set-StrictMode -Version 2 $ErrorActionPreference = "Stop" $ConfirmPreference = "None" -Set-Variable -Visibility Public -Option ReadOnly,AllScope,Constant -Name "AddressFamilies" -Value @( - [System.Net.Sockets.AddressFamily]::InterNetworkV6, - [System.Net.Sockets.AddressFamily]::InterNetwork -) +Set-Variable -Visibility Public -Option ReadOnly,AllScope,Constant -Name "AddressFamilies" -Value @{ + [System.Net.Sockets.AddressFamily]::InterNetworkV6 = 'IPv6' + [System.Net.Sockets.AddressFamily]::InterNetwork = 'IPv4' +} + $result = @{changed=$false} $params = Parse-Args -arguments $args -supports_check_mode $true @@ -43,6 +41,54 @@ Function Write-DebugLog { } } +Function Get-OptionalProperty { + <# + .SYNOPSIS + Retreives a property that may not exist from an object that may be null. + Optionally returns a default value. + Optionally coalesces to a new type with -as. + May return null, but will not throw. + #> + [CmdletBinding()] + param( + [Parameter(ValueFromPipeline=$true)] + [Object] + $InputObject , + + [Parameter(Mandatory=$true)] + [ValidateNotNullOrEmpty()] + [String] + $Name , + + [Parameter()] + [AllowNull()] + [Object] + $Default , + + [Parameter()] + [System.Type] + $As + ) + + Process { + if ($null -eq $InputObject) { + return $null + } + + $value = if ($InputObject.PSObject.Properties.Name -contains $Name) { + $InputObject.$Name + } else { + $Default + } + + if ($As) { + return $value -as $As + } + + return $value + } +} + Function Get-NetAdapterInfo { [CmdletBinding()] Param ( @@ -77,35 +123,25 @@ Function Get-NetAdapterInfo { $cim_params = @{ ClassName = "Win32_NetworkAdapterConfiguration" Filter = "InterfaceIndex = $($_.InterfaceIndex)" - Property = "DNSServerSearchOrder", "IPEnabled" + Property = "DNSServerSearchOrder", "IPEnabled", "SettingID" } - $adapter_config = Get-CimInstance @cim_params + $adapter_config = Get-CimInstance @cim_params | + Select-Object -Property DNSServerSearchOrder, IPEnabled, @{ + Name = 'InterfaceGuid' + Expression = { $_.SettingID } + } + if ($adapter_config.IPEnabled -eq $false) { return } - if (Get-Command -Name Get-DnsClientServerAddress -ErrorAction SilentlyContinue) { - $dns_servers = Get-DnsClientServerAddress -InterfaceIndex $_.InterfaceIndex | Select-Object -Property @( - "AddressFamily", - "ServerAddresses" - ) - } else { - $dns_servers = @( - [PSCustomObject]@{ - AddressFamily = [System.Net.Sockets.AddressFamily]::InterNetwork - ServerAddresses = $adapter_config.DNSServerSearchOrder - }, - [PSCustomObject]@{ - AddressFamily = [System.Net.Sockets.AddressFamily]::InterNetworkV6 - ServerAddresses = @() # WMI does not support IPv6 so we just keep it blank. - } - ) - } + $reg_info = $adapter_config | Get-RegistryNameServerInfo [PSCustomObject]@{ Name = $_.Name InterfaceIndex = $_.InterfaceIndex - DNSServers = $dns_servers + InterfaceGuid = $adapter_config.InterfaceGuid + RegInfo = $reg_info } } @@ -117,6 +153,63 @@ Function Get-NetAdapterInfo { } } +Function Get-RegistryNameServerInfo { + [CmdletBinding()] + Param ( + [Parameter(ValueFromPipeline=$true,ValueFromPipelineByPropertyName=$true,Mandatory=$true)] + [System.Guid] + $InterfaceGuid + ) + + Begin { + $protoItems = @{ + [System.Net.Sockets.AddressFamily]::InterNetwork = @{ + Interface = 'HKLM:\SYSTEM\CurrentControlSet\Services\Tcpip\Parameters\Interfaces\{{{0}}}' + StaticNameServer = 'NameServer' + DhcpNameServer = 'DhcpNameServer' + EnableDhcp = 'EnableDHCP' + } + + [System.Net.Sockets.AddressFamily]::InterNetworkV6 = @{ + Interface = 'HKLM:\SYSTEM\CurrentControlSet\Services\Tcpip6\Parameters\Interfaces\{{{0}}}' + StaticNameServer = 'NameServer' + DhcpNameServer = 'Dhcpv6DNSServers' + EnableDhcp = 'EnableDHCP' + } + } + } + + Process { + foreach ($addrFamily in $AddressFamilies.Keys) { + $items = $protoItems[$addrFamily] + $regPath = $items.Interface -f $InterfaceGuid + + if (($iface = Get-Item -LiteralPath $regPath -ErrorAction Ignore)) { + $iprop = $iface | Get-ItemProperty + $famInfo = @{ + AddressFamily = $addrFamily + UsingDhcp = Get-OptionalProperty -InputObject $iprop -Name $items.EnableDhcp -As bool + EffectiveNameServers = @() + DhcpAssignedNameServers = @() + NameServerBadFormat = $false + } + + if (($ns = Get-OptionalProperty -InputObject $iprop -Name $items.DhcpNameServer)) { + $famInfo.EffectiveNameServers = $famInfo.DhcpAssignedNameServers = $ns.Split(' ') + } + + if (($ns = Get-OptionalProperty -InputObject $iprop -Name $items.StaticNameServer)) { + $famInfo.EffectiveNameServers = $famInfo.StaticNameServers = $ns -split '[,;\ ]' + $famInfo.UsingDhcp = $false + $famInfo.NameServerBadFormat = $ns -match '[;\ ]' + } + + $famInfo + } + } + } +} + # minimal impl of Set-DnsClientServerAddress for 2008/2008R2 Function Set-DnsClientServerAddressLegacy { Param( @@ -155,35 +248,47 @@ Function Test-DnsClientMatch { ) Write-DebugLog ("Getting DNS config for adapter {0}" -f $AdapterInfo.Name) - $current_dns = [System.Net.IPAddress[]]($AdapterInfo.DNSServers.ServerAddresses) - Write-DebugLog ("Current DNS settings: {0}" -f ([string[]]$dns_servers -join ", ")) - - if(($null -eq $current_dns) -and ($null -eq $dns_servers)) { - Write-DebugLog "Neither are dns servers configured nor specified within the playbook." - return $true - } elseif ($null -eq $current_dns) { - Write-DebugLog "There are currently no dns servers specified, but they should be present." - return $false - } elseif ($null -eq $dns_servers) { - Write-DebugLog "There are currently dns servers specified, but they should be absent." - return $false - } - foreach($address in $current_dns) { - if($address -notin $dns_servers) { - Write-DebugLog "There are currently fewer dns servers present than specified within the playbook." - return $false + foreach ($proto in $AdapterInfo.RegInfo) { + $desired_dns = if ($dns_servers) { + $dns_servers | Where-Object -FilterScript {$_.AddressFamily -eq $proto.AddressFamily} } - } - foreach($address in $dns_servers) { - if($address -notin $current_dns) { - Write-DebugLog "There are currently further dns servers present than specified within the playbook." + + $current_dns = [System.Net.IPAddress[]]($proto.EffectiveNameServers) + Write-DebugLog ("Current DNS settings for '{1}' Address Family: {0}" -f ([string[]]$current_dns -join ", "),$AddressFamilies[$proto.AddressFamily]) + + if ($proto.NameServerBadFormat) { + Write-DebugLog "Malicious DNS server format detected. Will set DNS desired state." return $false + # See: https://www.welivesecurity.com/2016/06/02/crouching-tiger-hidden-dns/ } + + if ($proto.UsingDhcp -and -not $desired_dns) { + Write-DebugLog "DHCP DNS Servers are in use and no DNS servers were requested (DHCP is desired)." + } else { + if ($desired_dns -and -not $current_dns) { + Write-DebugLog "There are currently no DNS servers in use, but they should be present." + return $false + } + + if ($current_dns -and -not $desired_dns) { + Write-DebugLog "There are currently DNS servers in use, but they should be absent." + return $false + } + + if ($null -ne $current_dns -and + $null -ne $desired_dns -and + (Compare-Object -ReferenceObject $current_dns -DifferenceObject $desired_dns -SyncWindow 0)) { + Write-DebugLog "Static DNS servers are not in the desired state (incorrect or in the wrong order)." + return $false + } + } + + Write-DebugLog ("Current DNS settings match ({0})." -f ([string[]]$desired_dns -join ", ")) } - Write-DebugLog ("Current DNS settings match ({0})." -f ([string[]]$dns_servers -join ", ")) return $true } + Function Assert-IPAddress { Param([string] $address) diff --git a/lib/ansible/modules/windows/win_dns_client.py b/lib/ansible/modules/windows/win_dns_client.py index c42085fbeab..8c89cf70eb2 100644 --- a/lib/ansible/modules/windows/win_dns_client.py +++ b/lib/ansible/modules/windows/win_dns_client.py @@ -24,18 +24,21 @@ options: required: yes dns_servers: description: - - Single or ordered list of DNS servers (IPv4 and IPv6 addresses) to configure for lookup. An empty list will configure the adapter to use the - DHCP-assigned values on connections where DHCP is enabled, or disable DNS lookup on statically-configured connections. + - Single or ordered list of DNS servers (IPv4 and IPv6 addresses) to configure for lookup. + - An empty list will configure the adapter to use the DHCP-assigned values on connections where DHCP is enabled, + or disable DNS lookup on statically-configured connections. - IPv6 DNS servers can only be set on Windows Server 2012 or newer, older hosts can only set IPv4 addresses. - Before 2.10 use ipv4_addresses instead. type: list required: yes aliases: [ "ipv4_addresses", "ip_addresses", "addresses" ] notes: - - When setting an empty list of DNS server addresses on an adapter with DHCP enabled, a change will always be registered, since it is not possible to - detect the difference between a DHCP-sourced server value and one that is statically set. + - Before 2.10, when setting an empty list of DNS server addresses on an adapter with DHCP enabled, a change was always registered. + - In 2.10, DNS servers will always be reset if the format of nameservers in the registry is not comma delimited. + See U(https://www.welivesecurity.com/2016/06/02/crouching-tiger-hidden-dns/) author: - Matt Davis (@nitzmahone) +- Brian Scholer (@briantist) ''' EXAMPLES = r''' @@ -66,5 +69,4 @@ EXAMPLES = r''' ''' RETURN = r''' - ''' diff --git a/test/integration/targets/win_dns_client/tasks/main.yml b/test/integration/targets/win_dns_client/tasks/main.yml index 25d15914ed2..0e8b011b17d 100644 --- a/test/integration/targets/win_dns_client/tasks/main.yml +++ b/test/integration/targets/win_dns_client/tasks/main.yml @@ -177,6 +177,17 @@ - set_dhcp is changed - set_dhcp_actual.stdout_lines == [] +- name: reset IPv4 DNS back to DHCP (idempotent) + win_dns_client: + adapter_names: '{{ network_adapter_name }}' + ipv4_addresses: [] + register: set_dhcp_again + +- name: assert reset IPv4 DNS back to DHCP (idempotent) + assert: + that: + - set_dhcp_again is not changed + # Legacy WMI does not support setting IPv6 addresses so we can only test this on newer hosts that have the new cmdlets - name: check if server supports IPv6 win_shell: if (Get-Command -Name Get-NetAdapter -ErrorAction SilentlyContinue) { $true } else { $false }