From beb0fd9b8bd6b172d44916adb73e68949b585837 Mon Sep 17 00:00:00 2001
From: nwsparks <nsparks@outlook.com>
Date: Thu, 18 Jan 2018 19:00:35 -0500
Subject: [PATCH] win_iis_webbinding: Fix bug with ipaddress * returning
 multiple bindings (#34721)

* win_iis_webbinding: Fix bug with ipaddress * returning multiple bindings
instead of only the ones defined as *. Address possible future issues around
hostheader * by just disallowing it. Resolves 25473. Added new test for
this case.

Removed all validation for https binding collisions due to difficulty in
validating all cases in which they could or could not collide. As a
result, also removed return values relating to certificate data.

Updated testing and docs appropriately

* win_iis_webbinding: added break to remove binding loops
---
 .../modules/windows/win_iis_webbinding.ps1    | 188 +++++-------------
 .../modules/windows/win_iis_webbinding.py     |  33 +--
 .../library/test_get_webbindings.ps1          |  73 +++----
 .../win_iis_webbinding/tasks/failures.yml     |  68 +++----
 .../targets/win_iis_webbinding/tasks/http.yml |  55 -----
 .../win_iis_webbinding/tasks/setup.yml        |  13 +-
 6 files changed, 134 insertions(+), 296 deletions(-)

diff --git a/lib/ansible/modules/windows/win_iis_webbinding.ps1 b/lib/ansible/modules/windows/win_iis_webbinding.ps1
index 8f020c3196..94d827f94e 100644
--- a/lib/ansible/modules/windows/win_iis_webbinding.ps1
+++ b/lib/ansible/modules/windows/win_iis_webbinding.ps1
@@ -52,83 +52,38 @@ function Create-BindingInfo {
 }
 
 # Used instead of get-webbinding to ensure we always return a single binding
+# We can't filter properly with get-webbinding...ex get-webbinding ip * returns all bindings
 # pass it $binding_parameters hashtable
 function Get-SingleWebBinding {
-    $bind_search_splat = @{
-        'name' = $args[0].name
-        'protocol' = $args[0].protocol
-        'port' = $args[0].port
-        'ip' = $args[0].ip
-        'hostheader' = $args[0].hostheader
+
+    Try {
+        $site_bindings = get-webbinding -name $args[0].name
+    }
+    Catch {
+        # 2k8r2 throws this error when you run get-webbinding with no bindings in iis
+        If (-not $_.Exception.Message.CompareTo('Cannot process argument because the value of argument "obj" is null. Change the value of argument "obj" to a non-null value'))
+        {
+            Throw $_.Exception.Message
+        }
+        Else { return }
     }
 
-    # if no bindings exist, get-webbinding fails with an error that can't be ignored via error actions on older systems
-    # let's ignore that specific error
-    If (-not $bind_search_splat['hostheader'])
+    Foreach ($binding in $site_bindings)
     {
-        Try {
-            Get-WebBinding @bind_search_splat | Where-Object {$_.BindingInformation.Split(':')[-1] -eq [string]::Empty}
-        }
-        Catch {
-            If (-not $_.Exception.Message.CompareTo('Cannot process argument because the value of argument "obj" is null. Change the value of argument "obj" to a non-null value'))
-            {
-                Throw $_.Exception.Message
-            }
-        }
-    }
-    Else
-    {
-        Try {
-            Get-WebBinding @bind_search_splat
-        }
-        Catch {
-            If (-not $_.Exception.Message.CompareTo('Cannot process argument because the value of argument "obj" is null. Change the value of argument "obj" to a non-null value'))
-            {
-                Throw $_.Exception.Message
-            }
+        $splits = $binding.bindingInformation -split ':'
+
+        if (
+            $args[0].protocol -eq $binding.protocol -and
+            $args[0].ipaddress -eq $splits[0] -and
+            $args[0].port -eq $splits[1] -and
+            $args[0].hostheader -eq $splits[2]
+        )
+        {
+            Return $binding
         }
     }
 }
 
-Function Get-CertificateSubjects {
-Param (
-[string]$CertPath
-)
-    If (-Not (Test-Path $CertPath) )
-    {
-        Fail-Json -obj $result -message "Unable to locate certificate at $CertPath"
-    }
-
-    $cert = get-item $CertPath
-
-    If ([version][System.Environment]::OSVersion.Version -ge [version]6.2)
-    {
-        $cert.DnsNameList.unicode
-    }
-    Else
-    {
-        $san = $cert.extensions | Where-Object {$_.Oid.FriendlyName -eq 'Subject Alternative Name'}
-        If ($san)
-        {
-            $san.Format(1) -split '\r\n' | Where-Object {$_} | ForEach-Object {
-                ($_ -split '=')[-1]
-            }
-        }
-        Else
-        {
-            If ($cert.subject -like "*,*")
-            {
-                ($cert.Subject | Select-String "CN=(.*?),?").matches.groups[-1].value
-            }
-            Else
-            {
-                $cert.subject -replace "CN=",''
-            }
-        }
-    }
-}
-
-
 
 #############################
 ### Pre-Action Validation ###
@@ -203,29 +158,6 @@ If ($certificateHash -and $state -eq 'present')
     {
         Fail-Json -obj $result -message "Unable to locate certificate at $cert_path"
     }
-
-    #check if cert is wildcard and update results with useful info.
-    $cert_subjects = Get-CertificateSubjects $cert_path
-    $result.certificate_subjects = $cert_subjects
-    If ($cert_subjects | Where-Object {$_ -match '^\*'})
-    {
-        $cert_is_wildcard = $true
-        $result.cert_is_wildcard = $cert_is_wildcard
-    }
-    Else
-    {
-        $cert_is_wildcard = $false
-        $result.cert_is_wildcard = $cert_is_wildcard
-    }
-
-    If ($os_version -lt [version]6.2 -and $host_header -and -not $cert_is_wildcard)
-    {
-        Fail-Json -obj $result -message "You cannot specify host headers with SSL unless it is a wildcard certificate."
-    }
-    Elseif ($os_version -ge [version]6.2 -and $host_header -and (-not $cert_is_wildcard -and $sslFlags -eq 0))
-    {
-        Fail-Json -obj $result -message "You cannot specify host headers with SSL unless it is a wildcard certificate or SNI is enabled."
-    }
 }
 
 # make sure binding info is valid for central cert store if sslflags -gt 1
@@ -236,10 +168,10 @@ If ($sslFlags -gt 1 -and ($certificateHash -ne [string]::Empty -or $certificateS
     the certificate is automatically retrieved from the store rather than manually assigned to the binding."
 }
 
-# make sure host_header: '*' only present when state: absent
-If ($host_header -match '^\*$' -and $state -ne 'absent')
+# disallow host_header: '*'
+If ($host_header -eq '*')
 {
-    Fail-Json -obj $result -message "host_header: '*' can only be used in combinaiton with state: absent"
+    Fail-Json -obj $result -message "To make or remove a catch-all binding, please omit the host_header parameter entirely rather than specify host_header *"
 }
 
 ##########################
@@ -259,6 +191,10 @@ If ($host_header)
 {
     $binding_parameters.HostHeader = $host_header
 }
+Else
+{
+    $binding_parameters.HostHeader = [string]::Empty
+}
 
 # Get bindings matching parameters
 Try {
@@ -274,10 +210,27 @@ Catch {
 If ($current_bindings -and $state -eq 'absent')
 {
     Try {
-        # will remove multiple objects in the case of * host header
-        $current_bindings | Remove-WebBinding -WhatIf:$check_mode
+        #there is a bug in this method that will result in all bindings being removed if the IP in $current_bindings is a *
+        #$current_bindings | Remove-WebBinding -verbose -WhatIf:$check_mode
+
+        #another method that did not work. It kept failing to match on element and removed everything.
+        #$element = @{protocol="$protocol";bindingInformation="$ip`:$port`:$host_header"}
+        #Remove-WebconfigurationProperty -filter $current_bindings.ItemXPath -Name Bindings.collection -AtElement $element -WhatIf #:$check_mode
+
+        #this method works
+        [array]$bindings = Get-WebconfigurationProperty -filter $current_bindings.ItemXPath -Name Bindings.collection
+
+        $index = Foreach ($item in $bindings) {
+            If ( $protocol -eq $item.protocol -and $current_bindings.bindingInformation -eq $item.bindingInformation ) {
+                $bindings.indexof($item)
+                break
+            }
+        }
+
+        Remove-WebconfigurationProperty -filter $current_bindings.ItemXPath -Name Bindings.collection -AtIndex $index -WhatIf:$check_mode
         $result.changed = $true
     }
+
     Catch {
         Fail-Json -obj $result -message "Failed to remove the binding from IIS - $($_.Exception.Message)"
     }
@@ -299,7 +252,7 @@ ElseIf (-Not $current_bindings -and $state -eq 'absent')
 ### Modify existing bindings ###
 ################################
 <#
-since we have already.binding_info the parameters available to get-webbinding,
+since we have already have the parameters available to get-webbinding,
 we just need to check here for the ones that are not available which are the
 ssl settings (hash, store, sslflags). If they aren't set we update here, or
 exit with changed: false
@@ -373,44 +326,6 @@ ElseIf ($current_bindings)
 ########################
 ElseIf (-not $current_bindings -and $state -eq 'present')
 {
-    If ($certificateHash)
-    {
-        <#
-        Make sure a valid binding is specified. It's possible for another site to have a binding on the same IP:PORT. If
-        we bind to that same ip port without hostheader/sni it will cause a collision. Note, this check only matters for
-        https. Http will generate an error when new-webbinding is called if there is a conflict, unlike https.
-
-        I couldn't think of a good way to handle scenarios involving wildcards. There's just too many to think about and I
-        wouldn't want to potentially hard fail valid scenarios here that I did not consider...so those can still collide. We just skip
-        validation anytime an existing binding is a wildcard.
-
-        If a collision does occur, the website will be stopped. To help with this we'll return the website state into results.
-        #>
-
-        #use this instead of get-webbinding. on 2k8r2 get-webbinding fails with an error if a site with no bindings exists
-        $binding_matches = (Get-Website).bindings.collection | Where-Object {$_.BindingInformation -eq "$ip`:$port`:"}
-
-        #get dns names for all certs in matching bindings
-        $subjects = Foreach ($binding in $binding_matches)
-        {
-            $cert_path = "cert:\localmachine\$($binding.certificatestorename)\$($binding.certificatehash)"
-            Get-CertificateSubjects $cert_path
-        }
-
-        #skip validating scenarios where existing certs are wildcard
-        If (-not ($subjects | Where-Object {$_ -match "^\*"}))
-        {
-            If ($sslFlags -eq 0 -and $binding_matches -and $os_version -gt [version]6.2)
-            {
-                Fail-Json -obj $result -message "A conflicting binding has been found on the same ip $ip and port $port. To continue, you will either have to remove the offending binding or enable sni"
-            }
-            ElseIf ($binding_matches -and $os_version -lt [version]6.2)
-            {
-                Fail-Json -obj $result -message "A conflicting binding has been found on the same ip $ip and port $port. To continue you will need to remove the existing binding or assign a new IP or Port to this one"
-            }
-        }
-    }
-
     # add binding. this creates the binding, but does not apply a certificate to it.
     Try
     {
@@ -437,7 +352,8 @@ ElseIf (-not $current_bindings -and $state -eq 'present')
     If ($certificateHash -and -not $check_mode)
     {
         Try {
-            $new_binding = get-webbinding -Name $name -IPAddress $ip -port $port -Protocol $protocol -hostheader $host_header
+            #$new_binding = get-webbinding -Name $name -IPAddress $ip -port $port -Protocol $protocol -hostheader $host_header
+            $new_binding = Get-SingleWebBinding $binding_parameters
             $new_binding.addsslcertificate($certificateHash,$certificateStoreName)
         }
         Catch {
diff --git a/lib/ansible/modules/windows/win_iis_webbinding.py b/lib/ansible/modules/windows/win_iis_webbinding.py
index 1dd173bc26..5e74be940d 100644
--- a/lib/ansible/modules/windows/win_iis_webbinding.py
+++ b/lib/ansible/modules/windows/win_iis_webbinding.py
@@ -42,8 +42,7 @@ options:
   host_header:
     description:
       - The host header to bind to / use for the new site.
-      - For state absent, you can use c('*') here to remove all bindings for a particular
-        protocol/ip/port combination.
+      - If you are creating/removing a catch-all binding, omit this parameter rather than defining it as '*'.
   protocol:
     description:
       - The protocol to be used for the Web binding (usually HTTP, HTTPS, or FTP).
@@ -80,11 +79,19 @@ EXAMPLES = r'''
     port: 9090
     state: absent
 
+- name: Remove the default http binding
+  win_iis_webbinding:
+    name: Default Web Site
+    port: 80
+    ip: '*'
+    state: absent
+
 - name: Add a HTTPS binding
   win_iis_webbinding:
     name: Default Web Site
     protocol: https
     port: 443
+    ip: 127.0.0.1
     certificate_hash: B0D0FA8408FC67B230338FCA584D03792DA73F4C
     state: present
 
@@ -97,31 +104,9 @@ EXAMPLES = r'''
     ssl_flags: 1
     certificate_hash: D1A3AF8988FD32D1A3AF8988FD323792DA73F4C
     state: present
-
-- name: Remove all https bindings on port 443
-  win_iis_webbinding:
-    name: Default Web Site
-    protocol: https
-    port: 443
-    host_header: '*'
-    state: absent
 '''
 
 RETURN = r'''
-cert_is_wildcard:
-  description:
-    - Tells you if the certificate you are using is a wildcard
-  returned: when certificate_hash is defined
-  type: boolean
-  sample: false
-  version_added: "2.5"
-certificate_subjects:
-  description:
-    - All of the subject names for the certificate you are using
-  returned: when certificate_hash is defined
-  type: list
-  sample: ["*.test.com","test.com"]
-  version_added: "2.5"
 website_state:
   description:
     - The state of the website being targetted
diff --git a/test/integration/targets/win_iis_webbinding/library/test_get_webbindings.ps1 b/test/integration/targets/win_iis_webbinding/library/test_get_webbindings.ps1
index 084b6a1057..84ef10b75a 100644
--- a/test/integration/targets/win_iis_webbinding/library/test_get_webbindings.ps1
+++ b/test/integration/targets/win_iis_webbinding/library/test_get_webbindings.ps1
@@ -4,24 +4,18 @@
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
 #Requires -Module Ansible.ModuleUtils.Legacy
-#
+
 $params = Parse-Args -arguments $args -supports_check_mode $true
-$check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false
 
 $name = Get-AnsibleParam $params -name "name" -type str -failifempty $true -aliases 'website'
-#$state = Get-AnsibleParam $params "state" -default "present" -validateSet "present","absent"
 $host_header = Get-AnsibleParam $params -name "host_header" -type str
 $protocol = Get-AnsibleParam $params -name "protocol" -type str -default 'http'
 $port = Get-AnsibleParam $params -name "port" -type int -default '80'
 $ip = Get-AnsibleParam $params -name "ip" -default '*'
-$certificateHash = Get-AnsibleParam $params -name "certificate_hash" -type str
-$certificateStoreName = Get-AnsibleParam $params -name "certificate_store_name" -type str
-$sslFlags = Get-AnsibleParam $params -name "ssl_flags" -type int -default '0' -ValidateSet '0','1','2','3'
 
 $result = @{
   changed = $false
 }
-
 function Create-BindingInfo {
     $ht = @{
         'bindingInformation' = $args[0].bindingInformation
@@ -50,48 +44,41 @@ function Create-BindingInfo {
 # Used instead of get-webbinding to ensure we always return a single binding
 # pass it $binding_parameters hashtable
 function Get-SingleWebBinding {
-    $bind_search_splat = @{
-        'name' = $args[0].name
-        'protocol' = $args[0].protocol
-        'port' = $args[0].port
-        'ip' = $args[0].ip
-        'hostheader' = $args[0].hostheader
+
+    Try {
+        $site_bindings = get-webbinding -name $args[0].name
+    }
+    Catch {
+        # 2k8r2 throws this error when you run get-webbinding with no bindings in iis
+        If (-not $_.Exception.Message.CompareTo('Cannot process argument because the value of argument "obj" is null. Change the value of argument "obj" to a non-null value'))
+        {
+            Throw $_.Exception.Message
+        }
+        Else { return }
     }
 
-    # if no bindings exist, get-webbinding fails with an error that can't be ignored via error actions on older systems
-    # let's ignore that specific error
-    If (-not $bind_search_splat['hostheader'])
+    Foreach ($binding in $site_bindings)
     {
-        Try {
-            Get-WebBinding @bind_search_splat | Where-Object {$_.BindingInformation.Split(':')[-1] -eq [string]::Empty}
-        }
-        Catch {
-            If (-not $_.Exception.Message.CompareTo('Cannot process argument because the value of argument "obj" is null. Change the value of argument "obj" to a non-null value'))
-            {
-                Throw $_.Exception.Message
-            }
-        }
-    }
-    Else
-    {
-        Try {
-            Get-WebBinding @bind_search_splat
-        }
-        Catch {
-            If (-not $_.Exception.Message.CompareTo('Cannot process argument because the value of argument "obj" is null. Change the value of argument "obj" to a non-null value'))
-            {
-                Throw $_.Exception.Message
-            }
+        $splits = $binding.bindingInformation -split ':'
+
+        if (
+            $args[0].protocol -eq $binding.protocol -and
+            $args[0].ipaddress -eq $splits[0] -and
+            $args[0].port -eq $splits[1] -and
+            $args[0].hostheader -eq $splits[2]
+        )
+        {
+            Return $binding
         }
     }
 }
 
 # create binding search splat
 $binding_parameters = @{
-  Name = $name
-  Protocol = $protocol
-  Port = $port
-  IPAddress = $ip
+    Name = $name
+    Protocol = $protocol
+    Port = $port
+    IPAddress = $ip
 }
 
 # insert host header to search if specified, otherwise it will return * (all bindings matching protocol/ip)
@@ -99,6 +86,10 @@ If ($host_header)
 {
     $binding_parameters.HostHeader = $host_header
 }
+Else
+{
+    $binding_parameters.HostHeader = [string]::Empty
+}
 
 # Get bindings matching parameters
 Try {
@@ -119,4 +110,4 @@ If ($current_bindings)
 
     $result.binding = $binding_info
 }
-exit-json -obj $result
+exit-json -obj $result
\ No newline at end of file
diff --git a/test/integration/targets/win_iis_webbinding/tasks/failures.yml b/test/integration/targets/win_iis_webbinding/tasks/failures.yml
index 61753f5ab7..92736fe1bb 100644
--- a/test/integration/targets/win_iis_webbinding/tasks/failures.yml
+++ b/test/integration/targets/win_iis_webbinding/tasks/failures.yml
@@ -1,17 +1,12 @@
-- name: failure check bind with host header but no wc or sni
+- name: failure check define * for host header
   win_iis_webbinding:
     name: "{{ test_iis_site_name }}"
     state: present
-    host_header: test.com
-    protocol: https
+    host_header: '*'
+    protocol: http
     ip: '*'
-    port: 443
-    certificate_hash: "{{ thumbprint1.stdout_lines[0] }}"
-    ssl_flags: 0
   register: failure
-  failed_when:
-    - failure.msg != "You cannot specify host headers with SSL unless it is a wildcard certificate."
-    - failure.msg != "You cannot specify host headers with SSL unless it is a wildcard certificate or SNI is enabled."
+  failed_when: failure.msg != "To make or remove a catch-all binding, please omit the host_header parameter entirely rather than specify host_header *"
 
 - debug:
     var: failure
@@ -29,46 +24,47 @@
     with_items:
       - "{{ existing_sites.stdout_lines }}"
 
-  - name: add sites
+  - name: add testremove site
     win_iis_website:
-      name: "{{ item.name }}"
+      name: testremove
       state: started
-      ip: 127.0.0.1
-      port: "{{ item.port }}"
       physical_path: c:\inetpub\wwwroot
+
+  - name: add bindings to testremove
+    win_iis_webbinding:
+      name: testremove
+      ip: "{{ item.ip }}"
+      port: "{{ item.port }}"
     with_items:
-      - {name: testconflict1, port: 8080}
-      - {name: testconflict2, port: 8081}
+      - {ip: 127.0.0.1, port: 80}
+      - {ip: '*', port: 80}
 
-  - name: add https binding to testconflict1
+  - name: remove ip  * binding from testremove
     win_iis_webbinding:
-      name: testconflict1
-      state: present
-      protocol: https
-      port: 443
-      ip: 127.0.0.1
-      certificate_hash: "{{ thumbprint1.stdout_lines[0] }}"
+      name: testremove
+      state: absent
+      port: 80
+      ip: '*'
 
-  - name: add https binding to testconflict2 (expect failure)
-    win_iis_webbinding:
-      name: testconflict2
-      state: present
-      protocol: https
+  - name: get the remaining binding from testremove
+    test_get_webbindings:
+      name: testremove
+      port: 80
       ip: 127.0.0.1
-      port: 443
-      certificate_hash: "{{ thumbprint1.stdout_lines[0] }}"
-    register: failure
-    failed_when: '"A conflicting binding has been found on the same ip" not in failure.msg'
+    register: test_result
 
   - debug:
-      var: failure
+      var: test_result
       verbosity: 1
 
+  - name: assert that remove *:80 doesn't also remove 127.0.0.1:80
+    assert:
+      that:
+      - test_result.binding.ip == '127.0.0.1'
+      - test_result.binding.port == 80
+
   always:
   - name: remove websites
     win_iis_website:
-      name: "{{ item }}"
+      name: testremove
       state: absent
-    with_items:
-      - testconflict1
-      - testconflict2
diff --git a/test/integration/targets/win_iis_webbinding/tasks/http.yml b/test/integration/targets/win_iis_webbinding/tasks/http.yml
index 5b435068de..34c4cc2c19 100644
--- a/test/integration/targets/win_iis_webbinding/tasks/http.yml
+++ b/test/integration/targets/win_iis_webbinding/tasks/http.yml
@@ -315,58 +315,3 @@
     - http_header is not changed
     - http_header.binding_info is not defined
     - get_http_header.binding is not defined
-
-#bulk remove cm
-#add multiple bindings - verify they're present
-- name: bulk add http binding with header
-  win_iis_webbinding:
-    name: "{{ test_iis_site_name }}"
-    state: present
-    host_header: "{{ item }}"
-    protocol: http
-    ip: '*'
-    port: 80
-  register: http_header
-  with_items:
-   - test1.com
-   - test2.com
-   - test3.com
-
-- name: assert that 3 bindings were added
-  assert:
-    that:
-    - http_header is changed
-    - http_header | json_query('results[*].binding_info') | length == 3
-
-#cm remove with host_header: '*' - verify changed true and that bulk remove tries to get them all
-#remove with host_header: '*'
-
-- name: bulk remove http binding with header
-  win_iis_webbinding:
-    name: "{{ test_iis_site_name }}"
-    state: absent
-    host_header: '*'
-    protocol: http
-    ip: '*'
-    port: 80
-  register: http_header
-
-- name: get binding info header
-  test_get_webbindings:
-    name: "{{ test_iis_site_name }}"
-    host_header: "{{ item }}"
-    protocol: http
-    ip: '*'
-    port: 80
-  register: get_http_header
-  changed_when: false
-  with_items:
-   - test1.com
-   - test2.com
-   - test3.com
-
-- name: bulk remove assert that bindings are gone
-  assert:
-    that:
-    - http_header is changed
-    - http_header.binding_info | length == 3
diff --git a/test/integration/targets/win_iis_webbinding/tasks/setup.yml b/test/integration/targets/win_iis_webbinding/tasks/setup.yml
index 96e9cbd344..708f3edcf2 100644
--- a/test/integration/targets/win_iis_webbinding/tasks/setup.yml
+++ b/test/integration/targets/win_iis_webbinding/tasks/setup.yml
@@ -35,7 +35,6 @@
     state: absent
     protocol: "{{ item.protocol }}"
     port: "{{ item.port }}"
-    host_header: '*'
   with_items:
     - {protocol: http, port: 80}
     - {protocol: https, port: 443}
@@ -83,6 +82,12 @@
   raw: '(gci Cert:\LocalMachine\my | ? {$_.subject -eq "CN=*.test.com"})[0].Thumbprint'
   register: thumbprint_wc
 
-- debug: var=thumbprint1.stdout
-- debug: var=thumbprint2.stdout
-- debug: var=thumbprint_wc.stdout
+- debug: 
+    var: thumbprint1.stdout
+    verbosity: 1
+- debug: 
+    var: thumbprint2.stdout
+    verbosity: 1
+- debug: 
+    var: thumbprint_wc.stdout
+    verbosity: 1