[Bug] Handle half validated requests with multiple domains (#185)

* Add the info which test fails

* Fixing the half-validated bug

On certificates with multiple subject names (subject_alt_name), there is the possibility that a validation goes halfway through.

In such a case the filter_challenges filter plugin failed because it only searched in the authorizations field  when challenge_data field was empty. In a half-way validation the challenge_data will carry the tokens of the pending validations and therefore the find_challenges() fails on a expected_not_found validation.

* Adding test for a half-way through validation

* Fix linting

---------

Co-authored-by: Michael Kluge <michael.kluge@telekom.de>

Author: diLLec

plugins/filter/find_challenges.py

@@ -2,65 +2,104 @@
 
 
 def find_challenges(challenge_response: dict, challenge_type: str, expected_domains: list) -> dict:
-    challenges = build_challenges(challenge_response, challenge_type)
-
-    if set(challenges.keys()) == set(expected_domains):
+    """
+    1. check if all the expected domains are present in the challenge response
+    2. construct and return the following dictionary out of the API response
+       challenge_data and authorizations fields
+       {'<domain>': {[http-01/dns-01]: 'resource': <http path/dns record'> resource_value': <token>}}
+    """
+    challenges = build_challenges(challenge_response, challenge_type, expected_domains)
+    domains_in_response = find_domains_in_response(challenge_response, challenge_type)
+    errors = []
+
+    if domains_in_response == set(expected_domains):
         return challenges
     else:
-        errors = []
-        expected_not_found = set(expected_domains).difference(set(challenges.keys()))
+        expected_not_found = set(expected_domains).difference(domains_in_response)
         if expected_not_found:
             errors.append(f"Expected {challenge_type} challenges for the following domains not found: "
                           f"{','.join(expected_not_found)}")
 
-        unexpected_from_api = set(challenges.keys()).difference(set(expected_domains))
+        unexpected_from_api = domains_in_response.difference(set(expected_domains))
         if unexpected_from_api:
             errors.append(f"The API responded with {challenge_type} challenges for the following domains "
                           f"we did not expect: {','.join(unexpected_from_api)}")
+    if errors:
+        raise AnsibleError(" // ".join(errors))
+
+    return dict()
 
-        if errors:
-            raise AnsibleError(" // ".join(errors))
 
+def find_domains_in_response(challenge_response: dict, challenge_type: str) -> set:
+    """
+    find the domains in the response for which the API responded with a challenge of the specified challenge_type
+    """
+    domains = set()
+    for domain, domain_data in challenge_response['challenge_data'].items():
+        if challenge_type in domain_data:
+            domains.add(domain)
+
+    for domain, domain_data in challenge_response['authorizations'].items():
+        for challenge in domain_data['challenges']:
+            if challenge['type'] == challenge_type:
+                domains.add(domain)
+
+    return domains
 
-def build_challenges(challenge_response: dict, challenge_type: str) -> dict:
+
+def build_challenges(challenge_response: dict, challenge_type: str, expected_domains: list) -> dict:
+    """
+    try to extract the challenge for all expected_domains from the challenge_data field (first)0
+    and then from the authorization field, if not found
+    """
     if challenge_type not in ['http-01', 'dns-01']:
         raise AssertionError(f"Unknown challenge_type ({challenge_type})")
 
-    if len(challenge_response['challenge_data']) > 0:
-        return extract_challenges_from_challenge_data(challenge_response, challenge_type)
-    else:
-        return extract_challenges_from_authorizations_data(challenge_response, challenge_type)
+    extracted_challenges = {}
+    for domain in expected_domains:
+        extracted_challenges[domain] = {}
+        if domain in challenge_response['challenge_data']:
+            extracted_challenges[domain] = extract_challenge_from_challenge_data(domain, challenge_response,
+                                                                                 challenge_type)
+        elif domain in challenge_response['authorizations']:
+            extracted_challenges[domain] = extract_challenge_from_authorizations_data(domain, challenge_response,
+                                                                                      challenge_type)
 
+    return extracted_challenges
 
-def extract_challenges_from_challenge_data(challenge_response: dict, challenge_type: str) -> dict:
-    re = {}
-    for domain, domain_data in challenge_response['challenge_data'].items():
-        if challenge_type not in domain_data:
-            raise AssertionError(f"challenge_type '{challenge_type}' not in 'challenge_data'")
 
-        re[domain] = {}
-        re[domain][challenge_type] = domain_data[challenge_type]
+def extract_challenge_from_challenge_data(domain, challenge_response: dict, challenge_type: str) -> dict:
+    """
+    return the challenge data fields (record, resource, resource_value) out of the challenge_data field for domain
+    """
+    if challenge_type not in challenge_response['challenge_data'][domain]:
+        raise AssertionError(f"challenge_type '{challenge_type}' not in 'challenge_data'")
 
-    return re
+    return {challenge_type: challenge_response['challenge_data'][domain][challenge_type]}
 
 
-def extract_challenges_from_authorizations_data(challenge_response: dict, challenge_type: str):
-    re = {}
-    for domain, domain_data in challenge_response['authorizations'].items():
-        if 'challenges' not in challenge_response['authorizations'][domain]:
-            raise AssertionError(f"'challenges' missing for '{domain}' in API response")
+def extract_challenge_from_authorizations_data(domain, challenge_response: dict, challenge_type: str) -> dict:
+    """
+    return the challenge data fields from the authorizations field - note that the format is different from challange_data:
+    the authorizations field specifies the challenges with their type in a list instead of a dict
+    """
+    if 'challenges' not in challenge_response['authorizations'][domain]:
+        raise AssertionError(f"'challenges' missing in 'authorizations' field of domain '{domain}'")
 
-        for challenge in challenge_response['authorizations'][domain]['challenges']:
-            if challenge['type'] == challenge_type:
-                re[domain] = {}
-                if challenge_type == 'dns-01':
-                    re[domain][challenge_type] = build_data_from_authorizations_dns01(challenge['token'], domain)
-                elif challenge_type == 'http-01':
-                    re[domain][challenge_type] = build_data_from_authorizations_http01(challenge['token'], domain)
-    return re
+    for challenge in challenge_response['authorizations'][domain]['challenges']:
+        if challenge['type'] == challenge_type:
+            if challenge_type == 'dns-01':
+                return {challenge_type: build_data_from_authorizations_dns01(challenge['token'], domain)}
+            elif challenge_type == 'http-01':
+                return {challenge_type: build_data_from_authorizations_http01(challenge['token'], domain)}
+
+    raise AssertionError(f"challenge_type '{challenge_type}' not in 'authorizations'")
 
 
 def build_data_from_authorizations_dns01(token: str, domain: str) -> dict:
+    """
+    build the fields that one would expect out of challenge_data for data coming out of authorizations (for DNS-01)
+    """
     return {
         "record": f"_acme-challenge.{domain}",
         "resource": "_acme-challenge",
@@ -69,6 +108,9 @@ def build_data_from_authorizations_dns01(token: str, domain: str) -> dict:
 
 
 def build_data_from_authorizations_http01(token: str, domain: str) -> dict:
+    """
+    build the fields that one would expect out of challange_data for data coming out of authorizations (for HTTP-01)
+    """
     return {
         "resource": f".well-known/acme-challenge/{token}",
         "resource_value": token

tests/integration/targets/acme_letsencrypt/find-challenge-filter-plugin.yml

@@ -76,7 +76,7 @@
           ansible.builtin.set_fact:
             test_challenge: "{{ data_with_challenge_data | telekom_mms.acme.find_challenges('dns-01', ['blahtest1.example.org', 'blahtest2.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-01
           ansible.builtin.assert:
             that:
               - "set_fact_result is not failed"
@@ -93,22 +93,22 @@
           ansible.builtin.set_fact:
             test_challenge: "{{ data_with_challenge_data | telekom_mms.acme.find_challenges('http-01', ['blahtest1.example.org', 'blahtest2.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-02
           ansible.builtin.assert:
             that:
               - "set_fact_result is not failed"
               - "'blahtest1.example.org' in test_challenge and 'blahtest2.example.org' in test_challenge"
               - "data_with_challenge_data['challenge_data']['blahtest1.example.org']['http-01']['resource'] in test_challenge['blahtest1.example.org']['http-01'].resource"
               - "data_with_challenge_data['challenge_data']['blahtest1.example.org']['http-01']['resource_value'] in test_challenge['blahtest1.example.org']['http-01'].resource_value"
 
-    - name: TEST-03-1 default / dns-01 / expect less then provided
+    - name: TEST-03-1 default / dns-01 / expect less then provided by response
       block:
         - name: Set test_challenge
           ignore_errors: true
           ansible.builtin.set_fact:
             test_challenge: "{{ data_with_challenge_data | telekom_mms.acme.find_challenges('dns-01', ['blahtest1.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-03-1
           ansible.builtin.assert:
             that:
               - "set_fact_result is failed"
@@ -121,20 +121,20 @@
           ansible.builtin.set_fact:
             test_challenge: "{{ data_with_challenge_data | telekom_mms.acme.find_challenges('dns-01', ['blahtest1.example.org', 'blahtest2.example.org', 'one.more.is.one.too.many']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-03-2
           ansible.builtin.assert:
             that:
               - "set_fact_result is failed"
               - "'Expected dns-01 challenges for the following domains not found: one.more.is.one.too.many' in set_fact_result.msg"
 
-    - name: TEST-04 with_authorizations / dns-01 / 2 domains / data_with_challenge_data
+    - name: TEST-04-1 with_authorizations / dns-01 / 2 domains / data_with_challenge_data
       block:
         - name: Set test_challenge
           ignore_errors: true
           ansible.builtin.set_fact:
             test_challenge: "{{ data_with_challenge_data | telekom_mms.acme.find_challenges('dns-01', ['blahtest1.example.org', 'blahtest2.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-04-1
           ansible.builtin.assert:
             that:
               - "set_fact_result is not failed"
@@ -143,14 +143,14 @@
               - "data_with_challenge_data['challenge_data']['blahtest1.example.org']['dns-01']['resource'] in test_challenge['blahtest1.example.org']['dns-01'].resource"
               - "data_with_challenge_data['challenge_data']['blahtest1.example.org']['dns-01']['resource_value'] in test_challenge['blahtest1.example.org']['dns-01'].resource_value"
 
-    - name: TEST-04 with_authorizations / http-01 / 2 domains / data_with_challenge_data
+    - name: TEST-04-2 with_authorizations / http-01 / 2 domains / data_with_challenge_data
       block:
         - name: Set test_challenge
           ignore_errors: true
           ansible.builtin.set_fact:
             test_challenge: "{{ data_with_challenge_data | telekom_mms.acme.find_challenges('http-01', ['blahtest1.example.org', 'blahtest2.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-04-2
           ansible.builtin.assert:
             that:
               - "set_fact_result is not failed"
@@ -165,7 +165,7 @@
           ansible.builtin.set_fact:
             test_challenge: "{{ data_without_challenge_data | telekom_mms.acme.find_challenges('dns-01', ['blahtest1.example.org', 'blahtest2.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-05
           ansible.builtin.assert:
             that:
               - "set_fact_result is not failed"
@@ -179,7 +179,7 @@
           ansible.builtin.set_fact:
             test_challenge: "{{ data_without_challenge_data | telekom_mms.acme.find_challenges('http-01', ['blahtest1.example.org', 'blahtest2.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-06
           ansible.builtin.assert:
             that:
               - "set_fact_result is not failed"
@@ -193,7 +193,74 @@
           ansible.builtin.set_fact:
             test_challenge: "{{ data_without_challenge_data | telekom_mms.acme.find_challenges('http-01', ['blahtest1.example.org']) }}"
           register: set_fact_result
-        - name: Test assert
+        - name: Test assert TEST-07
           ansible.builtin.assert:
             that:
               - "set_fact_result is failed"
+
+    - name: TEST-08 with_mixed_challenge_and_authorizations / http-01
+      vars:
+        # blahtest1 is "valid" and therefore can't be found in the "challenge_data"
+        data_with_mixed_challenge_data:
+          account_uri: https://ACME.with_authorizations.de/v2/...
+          authorizations:
+            blahtest1.example.org:
+              challenges:
+                - status: valid
+                  token: Uwqt4U_l6lp04J2KW5nvgJ6LMPXvSrhr
+                  type: dns-01
+                  url: https://ACME.with_authorizations.de/v2/...
+                - status: valid
+                  token: ukTTEZIqv_KM9J0iEnysAaiiO31-Rpxg
+                  type: http-01
+                  url: https://ACME.with_authorizations.de/v2/....
+              expires: '2025-03-26T09:00:45Z'
+              identifier:
+                type: dns
+                value: blahtest1.example.org
+              status: pending
+              uri: https://ACME.with_authorizations.de/v2/...
+            blahtest2.example.org:
+              challenges:
+                - status: pending
+                  token: WfZ7M6gF63LZjxyqhXKyDkb3pg2NlvjFKHFTOdgd
+                  type: dns-01
+                  url: https://ACME.with_authorizations.de/v2/...
+                - status: pending
+                  token: ALKOBSIHfEBtdeDVeGj3zROeyECNNry9CD4pqwci
+                  type: http-01
+                  url: https://ACME.with_authorizations.de/v2/....
+              expires: '2025-03-26T09:00:45Z'
+              identifier:
+                type: dns
+                value: blahtest2.example.org
+              status: pending
+              uri: https://ACME.with_authorizations.de/v2/...
+          cert_days: -1
+          challenge_data:
+            blahtest2.example.org:
+              dns-01:
+                record: _acme-challenge.blahtest2.example.org
+                resource: _acme-challenge
+                resource_value: eksnHDhxBOzrTlOgiDzKlUXZtccjSwKIhc
+              http-01:
+                resource: .well-known/acme-challenge/ukTTEZIqv_KM9J0iEnysAaiiO31-Rpxg
+                resource_value: 8fV3Wc5hIqxMohz4llcqLqHuCv7WriMrgWYRogPypoTY5KRT84sfeU.8fV3Wc5hIqxMohz4llcqLqHuCv7WriMrgWYRogPypoTY5KRT84sfeU
+          challenge_data_dns:
+            _acme-challenge.blahtest1.example.org:
+              - bob4Yd3_SVv-7JET__3G5ZDVWaeGIOxAQmRf3SURbSY
+            _acme-challenge.blahtest2.example.org:
+              - eksnHDhxBOzrTlOgiDzKlUXZtccjSwKIhc
+      block:
+        - name: Set test_challenge
+          ignore_errors: true
+          ansible.builtin.set_fact:
+            test_challenge: "{{ data_with_mixed_challenge_data | telekom_mms.acme.find_challenges('http-01', ['blahtest1.example.org', 'blahtest2.example.org']) }}"
+          register: set_fact_result
+        - name: Test assert TEST-08
+          ansible.builtin.assert:
+            that:
+              - "set_fact_result is not failed"
+              - "'blahtest1.example.org' in test_challenge and 'blahtest2.example.org' in test_challenge"
+              - "data_with_mixed_challenge_data['authorizations']['blahtest1.example.org']['challenges'][1]['token'] in test_challenge['blahtest1.example.org']['http-01'].resource_value"
+              - "data_with_mixed_challenge_data['challenge_data']['blahtest2.example.org']['http-01'].resource_value in test_challenge['blahtest2.example.org']['http-01'].resource_value"