From 921d52b3608db7d2c9ce3dab0c64a12bb6726e14 Mon Sep 17 00:00:00 2001 From: csine-nflx Date: Fri, 13 Mar 2020 00:03:31 -0700 Subject: [PATCH 1/3] fixing get_dns_challenge() logic so duplicate domains (such as wildcard and not wildcard) do not match the wrong authorziations --- lemur/plugins/lemur_acme/plugin.py | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/lemur/plugins/lemur_acme/plugin.py b/lemur/plugins/lemur_acme/plugin.py index 91781966..e566352c 100644 --- a/lemur/plugins/lemur_acme/plugin.py +++ b/lemur/plugins/lemur_acme/plugin.py @@ -54,18 +54,30 @@ class AcmeHandler(object): current_app.logger.error(f"Unable to fetch DNS Providers: {e}") self.all_dns_providers = [] - def find_dns_challenge(self, host, authorizations): + def get_dns_challenges(self, host, authorizations): + """Get final domain to validate and dns challenges for it""" + + domain_to_validate, is_wildcard = self.strip_wildcard(host) dns_challenges = [] for authz in authorizations: - if not authz.body.identifier.value.lower() == host.lower(): + if not authz.body.identifier.value.lower() == domain_to_validate.lower(): + continue + if is_wildcard and not authz.body.wildcard: + continue + if not is_wildcard and authz.body.wildcard: continue for combo in authz.body.challenges: if isinstance(combo.chall, challenges.DNS01): dns_challenges.append(combo) - return dns_challenges - def maybe_remove_wildcard(self, host): - return host.replace("*.", "") + return domain_to_validate, dns_challenges + + def strip_wildcard(self, host): + """Removes the leading *. and returns Host and whether it was removed or not (True/False)""" + prefix = "*." + if host.startswith(prefix): + return host[len(prefix):], True + return host, False def maybe_add_extension(self, host, dns_provider_options): if dns_provider_options and dns_provider_options.get( @@ -86,9 +98,7 @@ class AcmeHandler(object): current_app.logger.debug("Starting DNS challenge for {0}".format(host)) change_ids = [] - - host_to_validate = self.maybe_remove_wildcard(host) - dns_challenges = self.find_dns_challenge(host_to_validate, order.authorizations) + host_to_validate, dns_challenges = self.get_dns_challenges(host, order.authorizations) host_to_validate = self.maybe_add_extension( host_to_validate, dns_provider_options ) @@ -325,7 +335,7 @@ class AcmeHandler(object): ) dns_provider_options = json.loads(dns_provider.credentials) account_number = dns_provider_options.get("account_id") - host_to_validate = self.maybe_remove_wildcard(authz_record.host) + host_to_validate, _ = self.strip_wildcard(authz_record.host) host_to_validate = self.maybe_add_extension( host_to_validate, dns_provider_options ) @@ -357,7 +367,7 @@ class AcmeHandler(object): dns_provider_options = json.loads(dns_provider.credentials) account_number = dns_provider_options.get("account_id") dns_challenges = authz_record.dns_challenge - host_to_validate = self.maybe_remove_wildcard(authz_record.host) + host_to_validate, _ = self.strip_wildcard(authz_record.host) host_to_validate = self.maybe_add_extension( host_to_validate, dns_provider_options ) From 1a19e250bb6561e52e3ab20bbb8dd9b82b36009b Mon Sep 17 00:00:00 2001 From: csine-nflx Date: Mon, 16 Mar 2020 11:24:17 -0700 Subject: [PATCH 2/3] updating and cleaning up tests --- lemur/plugins/lemur_acme/plugin.py | 7 +- lemur/plugins/lemur_acme/tests/test_acme.py | 105 +++++++++++--------- 2 files changed, 63 insertions(+), 49 deletions(-) diff --git a/lemur/plugins/lemur_acme/plugin.py b/lemur/plugins/lemur_acme/plugin.py index e566352c..3fc1df61 100644 --- a/lemur/plugins/lemur_acme/plugin.py +++ b/lemur/plugins/lemur_acme/plugin.py @@ -55,7 +55,7 @@ class AcmeHandler(object): self.all_dns_providers = [] def get_dns_challenges(self, host, authorizations): - """Get final domain to validate and dns challenges for it""" + """Get dns challenges for provided domain""" domain_to_validate, is_wildcard = self.strip_wildcard(host) dns_challenges = [] @@ -70,7 +70,7 @@ class AcmeHandler(object): if isinstance(combo.chall, challenges.DNS01): dns_challenges.append(combo) - return domain_to_validate, dns_challenges + return dns_challenges def strip_wildcard(self, host): """Removes the leading *. and returns Host and whether it was removed or not (True/False)""" @@ -98,7 +98,8 @@ class AcmeHandler(object): current_app.logger.debug("Starting DNS challenge for {0}".format(host)) change_ids = [] - host_to_validate, dns_challenges = self.get_dns_challenges(host, order.authorizations) + dns_challenges = self.get_dns_challenges(host, order.authorizations) + host_to_validate, _ = self.strip_wildcard(host) host_to_validate = self.maybe_add_extension( host_to_validate, dns_provider_options ) diff --git a/lemur/plugins/lemur_acme/tests/test_acme.py b/lemur/plugins/lemur_acme/tests/test_acme.py index 990a556e..eefd6c5b 100644 --- a/lemur/plugins/lemur_acme/tests/test_acme.py +++ b/lemur/plugins/lemur_acme/tests/test_acme.py @@ -23,11 +23,12 @@ class TestAcme(unittest.TestCase): } @patch("lemur.plugins.lemur_acme.plugin.len", return_value=1) - def test_find_dns_challenge(self, mock_len): + def test_get_dns_challenges(self, mock_len): assert mock_len from acme import challenges + host = "example.com" c = challenges.DNS01() mock_authz = Mock() @@ -35,9 +36,18 @@ class TestAcme(unittest.TestCase): mock_entry = Mock() mock_entry.chall = c mock_authz.body.resolved_combinations.append(mock_entry) - result = yield self.acme.find_dns_challenge(mock_authz) + result = yield self.acme.get_dns_challenges(host, mock_authz) self.assertEqual(result, mock_entry) + def test_strip_wildcard(self): + expected = ("example.com", False) + result = self.acme.strip_wildcard("example.com") + self.assertEqual(expected, result) + + expected = ("example.com", True) + result = self.acme.strip_wildcard("*.example.com") + self.assertEqual(expected, result) + def test_authz_record(self): a = plugin.AuthorizationRecord("host", "authz", "challenge", "id") self.assertEqual(type(a), plugin.AuthorizationRecord) @@ -45,9 +55,9 @@ class TestAcme(unittest.TestCase): @patch("acme.client.Client") @patch("lemur.plugins.lemur_acme.plugin.current_app") @patch("lemur.plugins.lemur_acme.plugin.len", return_value=1) - @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.find_dns_challenge") + @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.get_dns_challenges") def test_start_dns_challenge( - self, mock_find_dns_challenge, mock_len, mock_app, mock_acme + self, mock_get_dns_challenges, mock_len, mock_app, mock_acme ): assert mock_len mock_order = Mock() @@ -65,9 +75,12 @@ class TestAcme(unittest.TestCase): mock_dns_provider.create_txt_record = Mock(return_value=1) values = [mock_entry] - iterable = mock_find_dns_challenge.return_value + iterable = mock_get_dns_challenges.return_value iterator = iter(values) iterable.__iter__.return_value = iterator + + # mock_get_dns_challenges = Mock(return_value="") + result = self.acme.start_dns_challenge( mock_acme, "accountid", "host", mock_dns_provider, mock_order, {} ) @@ -102,7 +115,7 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.current_app") @patch("lemur.plugins.lemur_acme.cloudflare.wait_for_dns_change") def test_complete_dns_challenge_fail( - self, mock_wait_for_dns_change, mock_current_app, mock_acme + self, mock_wait_for_dns_change, mock_current_app, mock_acme ): mock_dns_provider = Mock() mock_dns_provider.wait_for_dns_change = Mock(return_value=True) @@ -127,15 +140,15 @@ class TestAcme(unittest.TestCase): @patch("acme.client.Client") @patch("OpenSSL.crypto", return_value="mock_cert") @patch("josepy.util.ComparableX509") - @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.find_dns_challenge") + @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.get_dns_challenges") @patch("lemur.plugins.lemur_acme.plugin.current_app") def test_request_certificate( - self, - mock_current_app, - mock_find_dns_challenge, - mock_jose, - mock_crypto, - mock_acme, + self, + mock_current_app, + mock_get_dns_challenges, + mock_jose, + mock_crypto, + mock_acme, ): mock_cert_response = Mock() mock_cert_response.body = "123" @@ -256,11 +269,11 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.cloudflare.current_app") @patch("lemur.plugins.lemur_acme.plugin.dns_provider_service") def test_get_dns_provider( - self, - mock_dns_provider_service, - mock_current_app_cloudflare, - mock_current_app_dyn, - mock_current_app, + self, + mock_dns_provider_service, + mock_current_app_cloudflare, + mock_current_app_dyn, + mock_current_app, ): provider = plugin.ACMEIssuerPlugin() route53 = provider.get_dns_provider("route53") @@ -278,14 +291,14 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.finalize_authorizations") @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.request_certificate") def test_get_ordered_certificate( - self, - mock_request_certificate, - mock_finalize_authorizations, - mock_get_authorizations, - mock_dns_provider_service, - mock_authorization_service, - mock_current_app, - mock_acme, + self, + mock_request_certificate, + mock_finalize_authorizations, + mock_get_authorizations, + mock_dns_provider_service, + mock_authorization_service, + mock_current_app, + mock_acme, ): mock_client = Mock() mock_acme.return_value = (mock_client, "") @@ -309,14 +322,14 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.finalize_authorizations") @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.request_certificate") def test_get_ordered_certificates( - self, - mock_request_certificate, - mock_finalize_authorizations, - mock_get_authorizations, - mock_dns_provider_service, - mock_authorization_service, - mock_current_app, - mock_acme, + self, + mock_request_certificate, + mock_finalize_authorizations, + mock_get_authorizations, + mock_dns_provider_service, + mock_authorization_service, + mock_current_app, + mock_acme, ): mock_client = Mock() mock_acme.return_value = (mock_client, "") @@ -349,14 +362,14 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.request_certificate") @patch("lemur.plugins.lemur_acme.plugin.authorization_service") def test_create_certificate( - self, - mock_authorization_service, - mock_request_certificate, - mock_finalize_authorizations, - mock_get_authorizations, - mock_current_app, - mock_dns_provider_service, - mock_acme, + self, + mock_authorization_service, + mock_request_certificate, + mock_finalize_authorizations, + mock_get_authorizations, + mock_current_app, + mock_dns_provider_service, + mock_acme, ): provider = plugin.ACMEIssuerPlugin() mock_authority = Mock() @@ -423,10 +436,10 @@ class TestAcme(unittest.TestCase): ultradns._post = Mock() ultradns._get = Mock() ultradns._get.return_value = {'zoneName': 'test.example.com.com', - 'rrSets': [{'ownerName': '_acme-challenge.test.example.com.', - 'rrtype': 'TXT (16)', 'ttl': 5, 'rdata': ['ABCDEFGHIJ']}], - 'queryInfo': {'sort': 'OWNER', 'reverse': False, 'limit': 100}, - 'resultInfo': {'totalCount': 1, 'offset': 0, 'returnedCount': 1}} + 'rrSets': [{'ownerName': '_acme-challenge.test.example.com.', + 'rrtype': 'TXT (16)', 'ttl': 5, 'rdata': ['ABCDEFGHIJ']}], + 'queryInfo': {'sort': 'OWNER', 'reverse': False, 'limit': 100}, + 'resultInfo': {'totalCount': 1, 'offset': 0, 'returnedCount': 1}} ultradns._delete = Mock() mock_metrics.send = Mock() ultradns.delete_txt_record(change_id, account_number, domain, token) From 07dc31bed7c92650f7ab5e5e3247bf36bebb2c52 Mon Sep 17 00:00:00 2001 From: csine-nflx Date: Mon, 16 Mar 2020 11:41:05 -0700 Subject: [PATCH 3/3] cleaning up whitespace changes --- lemur/plugins/lemur_acme/tests/test_acme.py | 49 ++++++++++----------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/lemur/plugins/lemur_acme/tests/test_acme.py b/lemur/plugins/lemur_acme/tests/test_acme.py index eefd6c5b..b2c32eec 100644 --- a/lemur/plugins/lemur_acme/tests/test_acme.py +++ b/lemur/plugins/lemur_acme/tests/test_acme.py @@ -78,9 +78,6 @@ class TestAcme(unittest.TestCase): iterable = mock_get_dns_challenges.return_value iterator = iter(values) iterable.__iter__.return_value = iterator - - # mock_get_dns_challenges = Mock(return_value="") - result = self.acme.start_dns_challenge( mock_acme, "accountid", "host", mock_dns_provider, mock_order, {} ) @@ -115,7 +112,7 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.current_app") @patch("lemur.plugins.lemur_acme.cloudflare.wait_for_dns_change") def test_complete_dns_challenge_fail( - self, mock_wait_for_dns_change, mock_current_app, mock_acme + self, mock_wait_for_dns_change, mock_current_app, mock_acme ): mock_dns_provider = Mock() mock_dns_provider.wait_for_dns_change = Mock(return_value=True) @@ -143,12 +140,12 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.get_dns_challenges") @patch("lemur.plugins.lemur_acme.plugin.current_app") def test_request_certificate( - self, - mock_current_app, - mock_get_dns_challenges, - mock_jose, - mock_crypto, - mock_acme, + self, + mock_current_app, + mock_get_dns_challenges, + mock_jose, + mock_crypto, + mock_acme, ): mock_cert_response = Mock() mock_cert_response.body = "123" @@ -291,14 +288,14 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.finalize_authorizations") @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.request_certificate") def test_get_ordered_certificate( - self, - mock_request_certificate, - mock_finalize_authorizations, - mock_get_authorizations, - mock_dns_provider_service, - mock_authorization_service, - mock_current_app, - mock_acme, + self, + mock_request_certificate, + mock_finalize_authorizations, + mock_get_authorizations, + mock_dns_provider_service, + mock_authorization_service, + mock_current_app, + mock_acme, ): mock_client = Mock() mock_acme.return_value = (mock_client, "") @@ -322,14 +319,14 @@ class TestAcme(unittest.TestCase): @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.finalize_authorizations") @patch("lemur.plugins.lemur_acme.plugin.AcmeHandler.request_certificate") def test_get_ordered_certificates( - self, - mock_request_certificate, - mock_finalize_authorizations, - mock_get_authorizations, - mock_dns_provider_service, - mock_authorization_service, - mock_current_app, - mock_acme, + self, + mock_request_certificate, + mock_finalize_authorizations, + mock_get_authorizations, + mock_dns_provider_service, + mock_authorization_service, + mock_current_app, + mock_acme, ): mock_client = Mock() mock_acme.return_value = (mock_client, "")