certificate_complete_chain: avoid infinite loops, and double roots wh…

…en root certificate was already part of chain (#360)

* Avoid infinite loops, and double roots when root certificate was already part of chain.

* Refactor tests for readability.

changelogs/fragments/360-certificate_complete_chain-loop.yml

@@ -0,0 +1,3 @@
+bugfixes:
+  - "certificate_complete_chain - do not hang when infinite loop is found (https://github.com/ansible-collections/community.crypto/issues/355, https://github.com/ansible-collections/community.crypto/pull/360)."
+  - "certificate_complete_chain - do not append root twice if the chain already ends with a root certificate (https://github.com/ansible-collections/community.crypto/pull/360)."

plugins/modules/certificate_complete_chain.py

@@ -238,12 +238,14 @@ def __init__(self, module):
         self.module = module
         self.certificates = set()
         self.certificate_by_issuer = dict()
+        self.certificate_by_cert = dict()
 
     def _load_file(self, path):
         certs = load_PEM_list(self.module, path, fail_on_error=False)
         for cert in certs:
             self.certificates.add(cert)
             self.certificate_by_issuer[cert.cert.subject] = cert
+            self.certificate_by_cert[cert.cert] = cert
 
     def load(self, path):
         '''
@@ -275,6 +277,16 @@ def format_cert(cert):
     return str(cert.cert)
 
 
+def check_cycle(module, occured_certificates, next):
+    '''
+    Make sure that next is not in occured_certificates so far, and add it.
+    '''
+    next_cert = next.cert
+    if next_cert in occured_certificates:
+        module.fail_json(msg='Found cycle while building certificate chain')
+    occured_certificates.add(next_cert)
+
+
 def main():
     module = AnsibleModule(
         argument_spec=dict(
@@ -313,13 +325,19 @@ def main():
     # Try to complete chain
     current = chain[-1]
     completed = []
+    occured_certificates = set([cert.cert for cert in chain])
+    if current.cert in roots.certificate_by_cert:
+        # Don't try to complete the chain when it's already ending with a root certificate
+        current = None
     while current:
         root = roots.find_parent(current)
         if root:
+            check_cycle(module, occured_certificates, root)
             completed.append(root)
             break
         intermediate = intermediates.find_parent(current)
         if intermediate:
+            check_cycle(module, occured_certificates, intermediate)
             completed.append(intermediate)
             current = intermediate
         else:

tests/integration/targets/certificate_complete_chain/tasks/main.yml

@@ -15,73 +15,144 @@
       src: '{{ role_path }}/files/'
       dest: '{{ remote_tmp_dir }}/files/'
 
-  - name: Find root for cert 1 using directory
-    certificate_complete_chain:
-      input_chain: '{{ lookup("file", "cert1-fullchain.pem", rstrip=False) }}'
-      root_certificates:
-      - '{{ remote_tmp_dir }}/files/roots/'
-    register: cert1_root
-  - name: Verify root for cert 1
-    assert:
-      that:
-      - cert1_root.complete_chain | join('') == (lookup('file', 'cert1.pem', rstrip=False) ~ lookup('file', 'cert1-chain.pem', rstrip=False) ~ lookup('file', 'cert1-root.pem', rstrip=False))
-      - cert1_root.root == lookup('file', 'cert1-root.pem', rstrip=False)
+  - block:
+    - name: Find root for cert 1 using directory
+      certificate_complete_chain:
+        input_chain: '{{ fullchain | trim }}'
+        root_certificates:
+        - '{{ remote_tmp_dir }}/files/roots/'
+      register: cert1_root
+    - name: Verify root for cert 1
+      assert:
+        that:
+        - cert1_root.complete_chain | join('') == (fullchain ~ root)
+        - cert1_root.root == root
+    vars:
+      fullchain: "{{ lookup('file', 'cert1-fullchain.pem', rstrip=False) }}"
+      root: "{{ lookup('file', 'cert1-root.pem', rstrip=False) }}"
 
-  - name: Find rootchain for cert 1 using intermediate and root PEM
-    certificate_complete_chain:
-      input_chain: '{{ lookup("file", "cert1.pem", rstrip=False) }}'
-      intermediate_certificates:
-      - '{{ remote_tmp_dir }}/files/cert1-chain.pem'
-      root_certificates:
-      - '{{ remote_tmp_dir }}/files/roots.pem'
-    register: cert1_rootchain
-  - name: Verify rootchain for cert 1
-    assert:
-      that:
-      - cert1_rootchain.complete_chain | join('') == (lookup('file', 'cert1.pem', rstrip=False) ~ lookup('file', 'cert1-chain.pem', rstrip=False) ~ lookup('file', 'cert1-root.pem', rstrip=False))
-      - cert1_rootchain.chain[:-1] | join('') == lookup('file', 'cert1-chain.pem', rstrip=False)
-      - cert1_rootchain.root == lookup('file', 'cert1-root.pem', rstrip=False)
+  - block:
+    - name: Find rootchain for cert 1 using intermediate and root PEM
+      certificate_complete_chain:
+        input_chain: '{{ cert }}'
+        intermediate_certificates:
+        - '{{ remote_tmp_dir }}/files/cert1-chain.pem'
+        root_certificates:
+        - '{{ remote_tmp_dir }}/files/roots.pem'
+      register: cert1_rootchain
+    - name: Verify rootchain for cert 1
+      assert:
+        that:
+        - cert1_rootchain.complete_chain | join('') == (cert ~ chain ~ root)
+        - cert1_rootchain.chain[:-1] | join('') == chain
+        - cert1_rootchain.root == root
+    vars:
+      cert: "{{ lookup('file', 'cert1.pem', rstrip=False) }}"
+      chain: "{{ lookup('file', 'cert1-chain.pem', rstrip=False) }}"
+      root: "{{ lookup('file', 'cert1-root.pem', rstrip=False) }}"
 
-  - name: Find root for cert 2 using directory
-    certificate_complete_chain:
-      input_chain: '{{ lookup("file", "cert2-fullchain.pem", rstrip=False) }}'
-      root_certificates:
-      - '{{ remote_tmp_dir }}/files/roots/'
-    register: cert2_root
-  - name: Verify root for cert 2
-    assert:
-      that:
-      - cert2_root.complete_chain | join('') == (lookup('file', 'cert2.pem', rstrip=False) ~ lookup('file', 'cert2-chain.pem', rstrip=False) ~ lookup('file', 'cert2-root.pem', rstrip=False))
-      - cert2_root.root == lookup('file', 'cert2-root.pem', rstrip=False)
+  - block:
+    - name: Find root for cert 2 using directory
+      certificate_complete_chain:
+        input_chain: "{{ fullchain | trim }}"
+        root_certificates:
+        - '{{ remote_tmp_dir }}/files/roots/'
+      register: cert2_root
+    - name: Verify root for cert 2
+      assert:
+        that:
+        - cert2_root.complete_chain | join('') == (fullchain ~ root)
+        - cert2_root.root == root
+    vars:
+      fullchain: "{{ lookup('file', 'cert2-fullchain.pem', rstrip=False) }}"
+      root: "{{ lookup('file', 'cert2-root.pem', rstrip=False) }}"
+
+  - block:
+    - name: Find rootchain for cert 2 using intermediate and root PEM
+      certificate_complete_chain:
+        input_chain: '{{ cert }}'
+        intermediate_certificates:
+        - '{{ remote_tmp_dir }}/files/cert2-chain.pem'
+        root_certificates:
+        - '{{ remote_tmp_dir }}/files/roots.pem'
+      register: cert2_rootchain
+    - name: Verify rootchain for cert 2
+      assert:
+        that:
+        - cert2_rootchain.complete_chain | join('') == (cert ~ chain ~ root)
+        - cert2_rootchain.chain[:-1] | join('') == chain
+        - cert2_rootchain.root == root
+    vars:
+      cert: "{{ lookup('file', 'cert2.pem', rstrip=False) }}"
+      chain: "{{ lookup('file', 'cert2-chain.pem', rstrip=False) }}"
+      root: "{{ lookup('file', 'cert2-root.pem', rstrip=False) }}"
+
+  - block:
+    - name: Find alternate rootchain for cert 2 using intermediate and root PEM
+      certificate_complete_chain:
+        input_chain: '{{ cert }}'
+        intermediate_certificates:
+        - '{{ remote_tmp_dir }}/files/cert2-altchain.pem'
+        root_certificates:
+        - '{{ remote_tmp_dir }}/files/roots.pem'
+      register: cert2_rootchain_alt
+    - name: Verify rootchain for cert 2
+      assert:
+        that:
+        - cert2_rootchain_alt.complete_chain | join('') == (cert ~ chain ~ root)
+        - cert2_rootchain_alt.chain[:-1] | join('') == chain
+        - cert2_rootchain_alt.root == root
+    vars:
+      cert: "{{ lookup('file', 'cert2.pem', rstrip=False) }}"
+      chain: "{{ lookup('file', 'cert2-altchain.pem', rstrip=False) }}"
+      root: "{{ lookup('file', 'cert2-altroot.pem', rstrip=False) }}"
+
+  - block:
+    - name: Find alternate rootchain for cert 2 when complete chain is already presented to the module
+      certificate_complete_chain:
+        input_chain: '{{ cert ~ chain ~ root }}'
+        root_certificates:
+        - '{{ remote_tmp_dir }}/files/roots.pem'
+      register: cert2_complete_chain
+    - name: Verify rootchain for cert 2
+      assert:
+        that:
+        - cert2_complete_chain.complete_chain | join('') == (cert ~ chain ~ root)
+        - cert2_complete_chain.chain == []
+        - cert2_complete_chain.root == root
+    vars:
+      cert: "{{ lookup('file', 'cert2.pem', rstrip=False) }}"
+      chain: "{{ lookup('file', 'cert2-altchain.pem', rstrip=False) }}"
+      root: "{{ lookup('file', 'cert2-altroot.pem', rstrip=False) }}"
 
-  - name: Find rootchain for cert 2 using intermediate and root PEM
+  - name: Check failure when no intermediate certificate can be found
     certificate_complete_chain:
-      input_chain: '{{ lookup("file", "cert2.pem", rstrip=False) }}'
+      input_chain: '{{ lookup("file", "cert2.pem", rstrip=True) }}'
       intermediate_certificates:
-      - '{{ remote_tmp_dir }}/files/cert2-chain.pem'
+      - '{{ remote_tmp_dir }}/files/cert1-chain.pem'
       root_certificates:
       - '{{ remote_tmp_dir }}/files/roots.pem'
-    register: cert2_rootchain
-  - name: Verify rootchain for cert 2
+    register: cert2_no_intermediate
+    ignore_errors: true
+  - name: Verify failure
     assert:
       that:
-      - cert2_rootchain.complete_chain | join('') == (lookup('file', 'cert2.pem', rstrip=False) ~ lookup('file', 'cert2-chain.pem', rstrip=False) ~ lookup('file', 'cert2-root.pem', rstrip=False))
-      - cert2_rootchain.chain[:-1] | join('') == lookup('file', 'cert2-chain.pem', rstrip=False)
-      - cert2_rootchain.root == lookup('file', 'cert2-root.pem', rstrip=False)
+      - cert2_no_intermediate is failed
+      - "cert2_no_intermediate.msg.startswith('Cannot complete chain. Stuck at certificate ')"
 
-  - name: Find alternate rootchain for cert 2 using intermediate and root PEM
+  - name: Check failure when infinite loop is found
     certificate_complete_chain:
-      input_chain: '{{ lookup("file", "cert2.pem", rstrip=True) }}'
+      input_chain: '{{ lookup("file", "cert2-fullchain.pem", rstrip=True) }}'
       intermediate_certificates:
-      - '{{ remote_tmp_dir }}/files/cert2-altchain.pem'
-      root_certificates:
       - '{{ remote_tmp_dir }}/files/roots.pem'
-    register: cert2_rootchain_alt
-  - name: Verify rootchain for cert 2
+      root_certificates:
+      - '{{ remote_tmp_dir }}/files/cert1-chain.pem'
+    register: cert2_infinite_loop
+    ignore_errors: true
+  - name: Verify failure
     assert:
       that:
-      - cert2_rootchain_alt.complete_chain | join('') == (lookup('file', 'cert2.pem', rstrip=False) ~ lookup('file', 'cert2-altchain.pem', rstrip=False) ~ lookup('file', 'cert2-altroot.pem', rstrip=False))
-      - cert2_rootchain_alt.chain[:-1] | join('') == lookup('file', 'cert2-altchain.pem', rstrip=False)
-      - cert2_rootchain_alt.root == lookup('file', 'cert2-altroot.pem', rstrip=False)
+      - cert2_infinite_loop is failed
+      - "cert2_infinite_loop.msg == 'Found cycle while building certificate chain'"
 
   when: cryptography_version.stdout is version('1.5', '>=')