Repository: ansible/ansible
Pull request: win_dns_client DHCP improvements + more
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
- Fixes #66450 : gets nameserver information from the registry so that DHCP status can be ensured. When both DHCP assigned nameservers and static nameservers are set in the registry, then static are used. When DHCP is enabled and static nameservers are not set, then DHCP assigned nameservers are used.
- Adds output fields for each adapter, returning useful information about the state of the nameservers.
- Addresses a malicious DNS server format in use by some malware, in which the effective static nameservers can be hidden and/or unsettable from the GUI. See: https://www.welivesecurity.com/2016/06/02/crouching-tiger-hidden-dns/

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME
win_dns_client

##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

<!--- Paste verbatim command output below, e.g. before and after your change -->


Problem statement:
win_dns_client should properly report status when setting DHCP
<!--- Verify first that your feature was not already discussed on GitHub -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
The `win_dns_client` module always shows "changed" when setting the adapter to use DHCP because the PowerShell cmdlets and WMI classes don't have a reliable way to determine that DHCP assigned nameservers are already in use.

A comment in the module suggests checking the registry.

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
win_dns_client

##### ADDITIONAL INFORMATION
<!--- Describe how the feature would be used, why it is needed and what it would solve -->
As is, it's not possible to know whether a change was actually made (or would be made in check mode).
<!--- Paste example playbooks or commands between quotes below -->

<!--- HINT: You can also paste gist.github.com links for larger files -->

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/windows/win_dns_client.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_dns_client.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @ShachafGoldstein @if-meaton @jborean93 @jhawkesworth @nitzmahone
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Address review feedback; dedup address-family code

Patch to review:
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 00000000000000..caddc30f583590
--- /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 2ca5294b18c2bd..dd9b4ee6615eb9 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,26 @@ Function Write-DebugLog {
     }
 }
 
+Function ConvertTo-AnsibleCompatibleObject {
+    <#
+        .SYNOPSIS
+        Adds a ContainsKey method to any object so Get-AnsibleParam can be used
+        to easily get and verify properties from it.
+    #>
+    [CmdletBinding()]
+    param(
+        [Parameter(ValueFromPipeline=$true)]
+        [Object]$Obj
+    )
+
+    Process {
+        $Obj | Add-Member -MemberType ScriptMethod -Name ContainsKey -Value {
+            param($key)
+            $this.PSObject.Properties.Name.Contains($key)
+        } -Force -PassThru
+    }
+}
+
 Function Get-NetAdapterInfo {
     [CmdletBinding()]
     Param (
@@ -77,35 +95,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 +125,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 | ConvertTo-AnsibleCompatibleObject
+                $famInfo = @{
+                    AddressFamily = $addrFamily
+                    UsingDhcp = Get-AnsibleParam -obj $iprop -name $items.EnableDhcp -type bool
+                    EffectiveNameServers = @()
+                    DhcpAssignedNameServers = @()
+                    NameServerBadFormat = $false
+                }
+
+                if (($ns = Get-AnsibleParam -obj $iprop -name $items.DhcpNameServer -type str)) {
+                    $famInfo.EffectiveNameServers = $famInfo.DhcpAssignedNameServers = $ns.Split(' ')
+                }
+
+                if (($ns = Get-AnsibleParam -obj $iprop -name $items.StaticNameServer -type str)) {
+                    $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 +220,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 c42085fbeab521..8c89cf70eb25c1 100644
--- a/lib/ansible/modules/windows/win_dns_client.py
+++ b/lib/ansible/modules/windows/win_dns_client.py
@@ -24,18 +24,21 @@
     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 @@
 '''
 
 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 25d15914ed2602..0e8b011b17d41e 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 }


Code review comments:
diff_hunk: @@ -43,6 +41,26 @@ Function Write-DebugLog {
     }
 }
 
+Function ConvertTo-AnsibleCompatibleObject {
+    <#
+        .SYNOPSIS
+        Adds a ContainsKey method to any object so Get-AnsibleParam can be used
+        to easily get and verify properties from it.
+    #>
+    [CmdletBinding()]
+    param(
+        [Parameter(ValueFromPipeline=$true)]
+        [Object]$Obj
+    )
+
+    Process {
+        $Obj | Add-Member -MemberType ScriptMethod -Name ContainsKey -Value {
+            param($key)
+            $this.PSObject.Properties.Name.Contains($key)
+        } -Force -PassThru
+    }
+}
path: lib/ansible/modules/windows/win_dns_client.ps1
text: @user1:
The Legacy module util is effectively read only and we should only be adding bug fixes and not new features to the module. The proper replacement is the `Ansible.Basic.cs` C# util which uses an interface that is similar to how Python modules work in Ansible (and adds a lot more features to a module).

diff_hunk: @@ -117,6 +125,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 | ConvertTo-AnsibleCompatibleObject
+                $famInfo = @{
+                    AddressFamily = $addrFamily
+                    UsingDhcp = Get-AnsibleParam -obj $iprop -name $items.EnableDhcp -type bool
path: lib/ansible/modules/windows/win_dns_client.ps1
text: @user1:
Avoid using `Get-AnsibleParam` here, while it might be convenient as a way of getting a property it means if we ever add deprecation notices or some other sort of sanity checks for older style module params.

With your `ConvertTo-AnsibleCompatibleObject` you've added a `ContainsKey()` method so at least you have a proper way of checking. But personally you know this is going to be a PSCustomObject so I think the abstraction you've added there isn't really necessary. You could just have your own custom `Get-AnsibleParams` like `Get-ContainerValue` or the like that checks if it has `ContainsKey()` or search the properties and return the "default" value you desire.

@author:
Ok sounds good, I'll fix these up a bit later and update the PR.

@author:
@user1 updated, thanks!