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
2020-02-17 00:35:54 -05:00 · 2020-02-17 00:35:54 -05:00 · be26f4916f
commit be26f4916f
parent e3d5dc0ed0
4 changed files with 177 additions and 53 deletions
--- a/changelogs/fragments/66451-win_dns_client-dhcp-support.yml
+++ 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
--- 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 ", "))
+    foreach ($proto in $AdapterInfo.RegInfo) {
+        $desired_dns = if ($dns_servers) {
+            $dns_servers | Where-Object -FilterScript {$_.AddressFamily -eq $proto.AddressFamily}
+        }

-    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."
+        $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/
        }
-    }
-    foreach($address in $dns_servers) {
-        if($address -notin $current_dns) {
-            Write-DebugLog "There are currently further dns servers present than specified within the playbook."
-            return $false
+
+        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)

--- 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'''
-
 '''
--- 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 }