Repository: ansible/ansible
Pull request:   python3 support for letsencrypt module (fixes #30690)
  * decorate fetch_url to upsert lowercase representation of the header names returned in info-dict

  * initialize result to a dict in some methods to prevent 'NoneType is not iterable' TypeError

  * use dict.get() to retrieve values from info dict to prevent KeyError

  * convert to/from text/bytes using _text methods for PY3 support

##### SUMMARY
fixes  #30690

##### ISSUE TYPE

 - Bugfix Pull Request


##### COMPONENT NAME
lib/ansible/modules/web_infrastructure/letsencrypt.py

##### ANSIBLE VERSION
```
ansible 2.5.0
```




Problem statement:
Lets Encrypt module appears to fail with Python 3
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

This appears to be the only report

##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME

lib/ansible/modules/web_infrastructure/letsencrypt.py

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
ansible 2.4.0.0
  config file = None
  configured module search path = [u'/home/ubuntu/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python2.7/dist-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.12 (default, Nov 19 2016, 06:48:10) [GCC 5.4.0 20160609]
```

##### CONFIGURATION

Ansible 2.4 command had no output

##### OS / ENVIRONMENT

Ubuntu 16.04

##### SUMMARY

Trying to write a role with Lets Encrypt that uses Python3 as the task executor, and it breaks. Various string encoding issues and so on.

##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: "Make the request of the Lets Encrypt API"
  letsencrypt:
    acme_directory: "{{ lets_encrypt_directory }}"
    account_email: "{{ lets_encrypt_account_email }}"
    account_key: "/etc/ssl/private/lets_encrypt.key"
    challenge: "{{ lets_encrypt_challenge_type }}"
    csr: "/etc/ssl/requests/{{ lets_encrypt_resource_name }}.csr"
    dest: "/etc/ssl/certs/{{ lets_encrypt_resource_name }}.crt"
  register: acme_data

- include: "providers/{{ lets_encrypt_challenge_type }}/{{ lets_encrypt_validation_provider }}.yaml"

- name: "Ask Lets Encrypt to validate and issue a new key"
  letsencrypt:
    acme_directory: "{{ lets_encrypt_directory }}"
    account_key: "/etc/ssl/private/lets_encrypt.key"
    challenge: "{{ lets_encrypt_challenge_type }}"
    csr: "/etc/ssl/requests/{{ lets_encrypt_resource_name }}.csr"
    dest: "/etc/ssl/certs/{{ lets_encrypt_resource_name }}.crt"
    data: "{{ acme_data }}"
```

##### EXPECTED RESULTS

It didn't work. This error particularly was fixed just by changing variable casing, but ran into other encoding errors.

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes below -->
```
The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py", line 812, in <module>
    main()
  File "/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py", line 802, in main
    client = ACMEClient(module)
  File "/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py", line 521, in __init__
    self.account = ACMEAccount(module)
  File "/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py", line 316, in __init__
    self.directory = ACMEDirectory(module)
  File "/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py", line 289, in __init__
    self.directory = simple_get(self.module, self.directory_root)
  File "/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py", line 191, in simple_get
    if info['content-type'].startswith('application/json'):
KeyError: 'content-type'

fatal: [default]: FAILED! => {
    "changed": false, 
    "failed": true, 
    "module_stderr": "Traceback (most recent call last):\n  File \"/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py\", line 812, in <module>\n    main()\n  File \"/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py\", line 802, in main\n    client = ACMEClient(module)\n  File \"/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py\", line 521, in __init__\n    self.account = ACMEAccount(module)\n  File \"/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py\", line 316, in __init__\n    self.directory = ACMEDirectory(module)\n  File \"/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py\", line 289, in __init__\n    self.directory = simple_get(self.module, self.directory_root)\n  File \"/tmp/ansible_f44oyk38/ansible_module_letsencrypt.py\", line 191, in simple_get\n    if info['content-type'].startswith('application/json'):\nKeyError: 'content-type'\n", 
    "module_stdout": "", 
    "msg": "MODULE FAILURE", 
    "rc": 0
}
```

Hints from issue discussion:
cc @dagwieers
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
\cc @mgruener  bug in the letsencrypt module

Commit message: fix dict comprehension sytax error in py2.6

Patch to review:
diff --git a/lib/ansible/modules/web_infrastructure/letsencrypt.py b/lib/ansible/modules/web_infrastructure/letsencrypt.py
index c1496cac78d519..3f7a39f85f6e9e 100644
--- a/lib/ansible/modules/web_infrastructure/letsencrypt.py
+++ b/lib/ansible/modules/web_infrastructure/letsencrypt.py
@@ -168,11 +168,27 @@
 import traceback
 from datetime import datetime
 
+from functools import wraps
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils._text import to_native
+from ansible.module_utils._text import to_native, to_text, to_bytes
 from ansible.module_utils.urls import fetch_url
 
 
+def _upsert_lowercase_headers(func):
+    '''
+     Decorator to append the lowercase representation of the header names (dict keys) returned in the info dict
+
+    '''
+    @wraps(func)
+    def func_wrapper(name, *args, **kwargs):
+        response, info = func(name, *args, **kwargs)
+        info.update(dict((header.lower(), value) for (header, value) in info.items()))
+        return response, info
+    return func_wrapper
+
+fetch_url = _upsert_lowercase_headers(fetch_url)
+
+
 def nopad_b64(data):
     return base64.urlsafe_b64encode(data).decode('utf8').replace("=", "")
 
@@ -180,12 +196,11 @@ def nopad_b64(data):
 def simple_get(module, url):
     resp, info = fetch_url(module, url, method='GET')
 
-    result = None
+    result = {}
     try:
         content = resp.read()
     except AttributeError:
-        if info['body']:
-            content = info['body']
+        content = info.get('body')
 
     if content:
         if info['content-type'].startswith('application/json'):
@@ -355,7 +370,7 @@ def _parse_account_key(self, key):
 
         pub_hex, pub_exp = re.search(
             r"modulus:\n\s+00:([a-f0-9\:\s]+?)\npublicExponent: ([0-9]+)",
-            out.decode('utf8'), re.MULTILINE | re.DOTALL).groups()
+            to_text(out), re.MULTILINE | re.DOTALL).groups()
         pub_exp = "{0:x}".format(int(pub_exp))
         if len(pub_exp) % 2:
             pub_exp = "0{0}".format(pub_exp)
@@ -385,16 +400,15 @@ def send_signed_request(self, url, payload):
             "header": self.jws_header,
             "protected": protected64,
             "payload": payload64,
-            "signature": nopad_b64(out),
+            "signature": nopad_b64(to_bytes(out)),
         })
 
         resp, info = fetch_url(self.module, url, data=data, method='POST')
-        result = None
+        result = {}
         try:
             content = resp.read()
         except AttributeError:
-            if info['body']:
-                content = info['body']
+            content = info.get('body')
 
         if content:
             if info['content-type'].startswith('application/json'):
@@ -540,10 +554,10 @@ def _get_csr_domains(self):
         _, out, _ = self.module.run_command(openssl_csr_cmd, check_rc=True)
 
         domains = set([])
-        common_name = re.search(r"Subject:.*? CN\s?=\s?([^\s,;/]+)", out.decode('utf8'))
+        common_name = re.search(r"Subject:.*? CN\s?=\s?([^\s,;/]+)", to_text(out))
         if common_name is not None:
             domains.add(common_name.group(1))
-        subject_alt_names = re.search(r"X509v3 Subject Alternative Name: \n +([^\n]+)\n", out.decode('utf8'), re.MULTILINE | re.DOTALL)
+        subject_alt_names = re.search(r"X509v3 Subject Alternative Name: \n +([^\n]+)\n", to_text(out), re.MULTILINE | re.DOTALL)
         if subject_alt_names is not None:
             for san in subject_alt_names.group(1).split(", "):
                 if san.startswith("DNS:"):
@@ -640,7 +654,7 @@ def _get_challenge_data(self, auth):
             elif type == 'dns-01':
                 # https://tools.ietf.org/html/draft-ietf-acme-acme-02#section-7.4
                 resource = '_acme-challenge'
-                value = nopad_b64(hashlib.sha256(keyauthorization).digest()).encode('utf8')
+                value = nopad_b64(hashlib.sha256(to_bytes(keyauthorization)).digest())
             else:
                 continue
 


Code review comments:
diff_hunk: @@ -168,24 +168,39 @@
 import traceback
 from datetime import datetime
 
+from functools import wraps
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils._text import to_native
+from ansible.module_utils._text import to_native, to_text, to_bytes
 from ansible.module_utils.urls import fetch_url
 
 
+def _upsert_lowercase_headers(func):
+    '''
+     Decorator to append the lowercase representation of the header names (dict keys) returned in the info dict
+
+    '''
+    @wraps(func)
+    def func_wrapper(name, *args, **kwargs):
+        response, info = func(name, *args, **kwargs)
+        info.update(dict((header.lower(), value) for (header, value) in info.items()))
+        return response, info
+    return func_wrapper
+
+fetch_url = _upsert_lowercase_headers(fetch_url)
path: lib/ansible/modules/web_infrastructure/letsencrypt.py
text: @user1:
I would just make this a plain function that calls fetch_url and then lowercases the keys instead of making it a decorator.  I'm a little hesitant about overriding the name (I'd use a different name than fetch_url) but I can see you do that so you don't have to change the name everywhere.  Since I'm not the module author, I won't block it over that but I think it's better style to use a new name.

@author:
The reason I've used a decorator was indeed to prevent having to rename all fetch_url occurrences. And since I'm not altering the values but merely ensuring the lowercase representation is included, I figured a decorator would be easier. 
Maybe this should be fixed in the fetch_url module utility ? Otherwise every module that uses this function AND wants to support py3 needs to create a wrapper or create several conditionals to capture header case variations ?

diff_hunk: @@ -355,7 +370,7 @@ def _parse_account_key(self, key):
 
         pub_hex, pub_exp = re.search(
             r"modulus:\n\s+00:([a-f0-9\:\s]+?)\npublicExponent: ([0-9]+)",
-            out.decode('utf8'), re.MULTILINE | re.DOTALL).groups()
+            to_text(out), re.MULTILINE | re.DOTALL).groups()
path: lib/ansible/modules/web_infrastructure/letsencrypt.py
text: @user1:
Unless simply constructing a message for the user, use to_text, to_bytes, etc with errors='surrogate_or_strict' like this:

``` python
to_text(out, errors='surrogate_or_strict'), re.MULTILINE | re.DOTALL).groups()
```

@author:
Ok, have added the error handler argument.