From b1c097addb044fdad8bed5063b82a6e9bcfc62e2 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Tue, 4 Jun 2019 16:25:14 +0200 Subject: [PATCH 1/7] salt/modules/acme.py: Fix function docstrings. Fix handling of "window" parameter in "needs_renewal". Pylint-inspired layout fixes. --- salt/modules/acme.py | 45 ++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/salt/modules/acme.py b/salt/modules/acme.py index 792101231abd..8136c7e1cd76 100644 --- a/salt/modules/acme.py +++ b/salt/modules/acme.py @@ -42,6 +42,7 @@ # Import salt libs import salt.utils.path +from salt.exceptions import SaltInvocationError log = logging.getLogger(__name__) @@ -77,7 +78,7 @@ def _expires(name): cert_file = _cert_file(name, 'cert') # Use the salt module if available if 'tls.cert_info' in __salt__: - expiry = __salt__['tls.cert_info'](cert_file)['not_after'] + expiry = __salt__['tls.cert_info'](cert_file).get('not_after', 0) # Cobble it together using the openssl binary else: openssl_cmd = 'openssl x509 -in {0} -noout -enddate'.format(cert_file) @@ -93,8 +94,8 @@ def _renew_by(name, window=None): ''' Date before a certificate should be renewed - :param name: Common Name of the certificate (DNS name of certificate) - :param window: days before expiry date to renew + :param str name: Common Name of the certificate (DNS name of certificate) + :param int window: days before expiry date to renew :return datetime object of first renewal date ''' expiry = _expires(name) @@ -161,7 +162,8 @@ def cert(name, .. code-block:: bash - salt 'gitlab.example.com' acme.cert dev.example.com "[gitlab.example.com]" test_cert=True renew=14 webroot=/opt/gitlab/embedded/service/gitlab-rails/public + salt 'gitlab.example.com' acme.cert dev.example.com "[gitlab.example.com]" test_cert=True \ + renew=14 webroot=/opt/gitlab/embedded/service/gitlab-rails/public ''' cmd = [LEA, 'certonly', '--non-interactive', '--agree-tos'] @@ -235,9 +237,13 @@ def cert(name, cmd.append('--expand') res = __salt__['cmd.run_all'](' '.join(cmd)) if res['retcode'] != 0: - return {'result': False, 'comment': 'Certificate {0} renewal failed with:\n{1}'.format(name, res['stderr'])} + return {'result': False, + 'comment': ('Certificate {0} renewal failed with:\n{1}' + ''.format(name, res['stderr']))} else: - return {'result': False, 'comment': 'Certificate {0} renewal failed with:\n{1}'.format(name, res['stderr'])} + return {'result': False, + 'comment': ('Certificate {0} renewal failed with:\n{1}' + ''.format(name, res['stderr']))} if 'no action taken' in res['stdout']: comment = 'Certificate {0} unchanged'.format(cert_file) @@ -268,7 +274,7 @@ def certs(): salt 'vhost.example.com' acme.certs ''' - return __salt__['file.readdir'](LE_LIVE)[2:] + return [item for item in __salt__['file.readdir'](LE_LIVE)[2:] if os.path.isdir(item)] def info(name): @@ -278,7 +284,7 @@ def info(name): .. note:: Will output tls.cert_info if that's available, or OpenSSL text if not - :param name: CommonName of cert + :param str name: CommonName of certificate CLI example: @@ -286,6 +292,8 @@ def info(name): salt 'gitlab.example.com' acme.info dev.example.com ''' + if not has(name): + return None cert_file = _cert_file(name, 'cert') # Use the salt module if available if 'tls.cert_info' in __salt__: @@ -304,7 +312,7 @@ def expires(name): ''' The expiry date of a certificate in ISO format - :param name: CommonName of cert + :param str name: CommonName of certificate CLI example: @@ -319,7 +327,7 @@ def has(name): ''' Test if a certificate is in the Let's Encrypt Live directory - :param name: CommonName of cert + :param str name: CommonName of certificate Code example: @@ -335,8 +343,8 @@ def renew_by(name, window=None): ''' Date in ISO format when a certificate should first be renewed - :param name: CommonName of cert - :param window: number of days before expiry when renewal should take place + :param str name: CommonName of certificate + :param int window: number of days before expiry when renewal should take place ''' return _renew_by(name, window).isoformat() @@ -345,8 +353,8 @@ def needs_renewal(name, window=None): ''' Check if a certificate needs renewal - :param name: CommonName of cert - :param window: Window in days to renew earlier or True/force to just return True + :param str name: CommonName of certificate + :param bool/str/int window: Window in days to renew earlier or True/force to just return True Code example: @@ -360,4 +368,13 @@ def needs_renewal(name, window=None): if window is not None and window in ('force', 'Force', True): return True + if window: + if not (isinstance(window, int) or (hasattr(window, 'isdigit') and window.isdigit())): + raise SaltInvocationError( + 'The argument "window", if provided, must be one of the following : ' + 'True (boolean), "force" or "Force" (str) or a numerical value in days.' + ) + else: + window = int(window) + return _renew_by(name, window) <= datetime.datetime.today() From 28f9534d3d9f7f426d487ec795d1818269c09bf0 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Tue, 4 Jun 2019 16:26:01 +0200 Subject: [PATCH 2/7] salt/states/acme.py: Add proper handling of testmode. Add proper detection if nothing/fetch/renew is needed to be done. --- salt/states/acme.py | 114 +++++++++++++++++++------------------------- 1 file changed, 48 insertions(+), 66 deletions(-) diff --git a/salt/states/acme.py b/salt/states/acme.py index 9ecc1fee284d..3ae7a508ba93 100644 --- a/salt/states/acme.py +++ b/salt/states/acme.py @@ -29,6 +29,10 @@ from __future__ import absolute_import, print_function, unicode_literals import logging +# Import salt libs +import salt.utils.dictdiffer +import salt.ext.six as six + log = logging.getLogger(__name__) @@ -88,74 +92,52 @@ def cert(name, :param dns_plugin: Name of a DNS plugin to use (currently only 'cloudflare') :param dns_plugin_credentials: Path to the credentials file if required by the specified DNS plugin ''' + ret = {'name': name, 'result': 'changeme', 'comment': [], 'changes': {}} + action = None - if __opts__['test']: - ret = { - 'name': name, - 'changes': {}, - 'result': None - } - window = None - try: - window = int(renew) - except Exception: - pass - - comment = 'Certificate {0} '.format(name) - if not __salt__['acme.has'](name): - comment += 'would have been obtained' - elif __salt__['acme.needs_renewal'](name, window): - comment += 'would have been renewed' - else: - comment += 'would not have been touched' - ret['result'] = True - ret['comment'] = comment - return ret - + current_certificate = {} + new_certificate = {} if not __salt__['acme.has'](name): - old = None - else: - old = __salt__['acme.info'](name) - - res = __salt__['acme.cert']( - name, - aliases=aliases, - email=email, - webroot=webroot, - certname=certname, - test_cert=test_cert, - renew=renew, - keysize=keysize, - server=server, - owner=owner, - group=group, - mode=mode, - preferred_challenges=preferred_challenges, - tls_sni_01_port=tls_sni_01_port, - tls_sni_01_address=tls_sni_01_address, - http_01_port=http_01_port, - http_01_address=http_01_address, - dns_plugin=dns_plugin, - dns_plugin_credentials=dns_plugin_credentials, - ) - - ret = { - 'name': name, - 'result': res['result'] is not False, - 'comment': res['comment'] - } - - if res['result'] is None: - ret['changes'] = {} + action = 'obtain' + elif __salt__['acme.needs_renewal'](name, renew): + action = 'renew' + current_certificate = __salt__['acme.info'](name) else: - if not __salt__['acme.has'](name): - new = None + ret['result'] = True + ret['comment'].append('Certificate {} exists and does not need renewal.' + ''.format(name)) + + if action: + if __opts__['test']: + ret['result'] = None + ret['comment'].append('Certificate {} would have been {}ed.' + ''.format(name, action)) + ret['changes'] = {'old': 'current certificate', 'new': 'new certificate'} else: - new = __salt__['acme.info'](name) - - ret['changes'] = { - 'old': old, - 'new': new - } - + res = __salt__['acme.cert']( + name, + aliases=aliases, + email=email, + webroot=webroot, + certname=certname, + test_cert=test_cert, + renew=renew, + keysize=keysize, + server=server, + owner=owner, + group=group, + mode=mode, + preferred_challenges=preferred_challenges, + tls_sni_01_port=tls_sni_01_port, + tls_sni_01_address=tls_sni_01_address, + http_01_port=http_01_port, + http_01_address=http_01_address, + dns_plugin=dns_plugin, + dns_plugin_credentials=dns_plugin_credentials, + ) + ret['result'] = res['result'] + ret['comment'].append(res['comment']) + if ret['result']: + new_certificate = __salt__['acme.info'](name) + ret['changes'] = salt.utils.dictdiffer.deep_diff(current_certificate, new_certificate) return ret From 3c209dcb57876911b08931e7f6a673495242d606 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Tue, 4 Jun 2019 16:26:41 +0200 Subject: [PATCH 3/7] Added unit tests for acme module and state. --- tests/unit/modules/test_acme.py | 221 ++++++++++++++++++++++++++++++++ tests/unit/states/test_acme.py | 128 ++++++++++++++++++ 2 files changed, 349 insertions(+) create mode 100644 tests/unit/modules/test_acme.py create mode 100644 tests/unit/states/test_acme.py diff --git a/tests/unit/modules/test_acme.py b/tests/unit/modules/test_acme.py new file mode 100644 index 000000000000..4d1d4b10c057 --- /dev/null +++ b/tests/unit/modules/test_acme.py @@ -0,0 +1,221 @@ +# -*- coding: utf-8 -*- +''' +:codeauthor: Herbert Buurman +''' + +# Import future libs +from __future__ import absolute_import, print_function, unicode_literals + +# Import Python libs +import textwrap +import datetime + +# Import Salt Testing libs +from tests.support.mixins import LoaderModuleMockMixin +from tests.support.unit import skipIf, TestCase +from tests.support.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch + +# Import Salt Module +import salt.modules.acme as acme +import salt.utils.dictupdate +from salt.exceptions import SaltInvocationError + + +@skipIf(NO_MOCK, NO_MOCK_REASON) +class AcmeTestCase(TestCase, LoaderModuleMockMixin): + ''' + Test cases for salt.modules.acme + ''' + + def setup_loader_modules(self): + return {acme: {}} + + def test_certs(self): + ''' + Test listing certs + ''' + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'file.readdir': MagicMock(return_value=['.', '..', 'README', 'test_expired', 'test_valid']) + }), \ + patch('os.path.isdir', side_effect=[False, True, True]): + self.assertEqual(acme.certs(), ['test_expired', 'test_valid']) + + def test_has(self): + ''' + Test checking if certificate (does not) exist. + ''' + with patch.dict(acme.__salt__, {'file.file_exists': MagicMock(return_value=True)}): # pylint: disable=no-member + self.assertTrue(acme.has('test_expired')) + with patch.dict(acme.__salt__, {'file.file_exists': MagicMock(return_value=False)}): # pylint: disable=no-member + self.assertFalse(acme.has('test_invalid')) + + def test_needs_renewal(self): + ''' + Test if expired certs do indeed need renewal. + ''' + expired = datetime.date.today() - datetime.timedelta(days=3) - datetime.date(1970, 1, 1) + valid = datetime.date.today() + datetime.timedelta(days=3) - datetime.date(1970, 1, 1) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value={'not_after': expired.total_seconds()}) + }): + self.assertTrue(acme.needs_renewal('test_expired')) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value={'not_after': valid.total_seconds()}) + }): + self.assertFalse(acme.needs_renewal('test_valid')) + # Test with integer window parameter + self.assertTrue(acme.needs_renewal('test_valid', window=5)) + # Test with string-like window parameter + self.assertTrue(acme.needs_renewal('test_valid', window='5')) + # Test with invalid window parameter + self.assertRaises(SaltInvocationError, acme.needs_renewal, 'test_valid', window='foo') + + def test_expires(self): + ''' + Test if expires function functions properly. + ''' + test_value = datetime.datetime.today() - datetime.timedelta(days=3) + test_stamp = test_value - datetime.datetime(1970, 1, 1) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value={'not_after': test_stamp.total_seconds()}) + }): + self.assertEqual( + acme.expires('test_expired'), + datetime.datetime.fromtimestamp(test_stamp.total_seconds()).isoformat() + ) + + def test_info(self): + ''' + Test certificate information retrieval. + ''' + certinfo_result = { + "not_after": 1559471377, + "signature_algorithm": "sha256WithRSAEncryption", + "extensions": {}, + "fingerprint": ("FB:A4:5F:71:D6:5D:6C:B6:1D:2C:FD:91:09:2C:1C:52:" + "3C:EC:B6:4D:1A:95:65:37:04:D0:E2:5E:C7:64:0C:9C"), + "serial_number": 6461481982668892235, + "issuer": {}, + "not_before": 1559557777, + "subject": {}, + } + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'tls.cert_info': MagicMock(return_value=certinfo_result), + 'file.file_exists': MagicMock(return_value=True) + }): + self.assertEqual(acme.info('test'), certinfo_result) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run': MagicMock(return_value='foo'), + 'file.file_exists': MagicMock(return_value=True) + }): + self.assertEqual(acme.info('test'), 'foo') + + def test_cert(self): + ''' + Test certificate retrieval/renewal + ''' + valid_timestamp = (datetime.datetime.now() + datetime.timedelta(days=30) - + datetime.datetime(1970, 1, 1, 0, 0, 0, 0)).total_seconds() + expired_timestamp = (datetime.datetime.now() - datetime.timedelta(days=3) - + datetime.datetime(1970, 1, 1, 0, 0, 0, 0)).total_seconds() + cmd_new_cert = { + 'stdout': textwrap.dedent('''IMPORTANT NOTES: + - Congratulations! Your certificate and chain have been saved at: + /etc/letsencrypt/live/test/fullchain.pem + Your key file has been saved at: + /etc/letsencrypt/live/test/privkey.pem + Your cert will expire on 2019-08-07. To obtain a new or tweaked + version of this certificate in the future, simply run certbot + again. To non-interactively renew *all* of your certificates, run + "certbot renew" + - If you like Certbot, please consider supporting our work by: + + Donating to ISRG / Let's Encrypt: https://letsencrypt.org/donate + Donating to EFF: https://eff.org/donate-le + '''), + 'stderr': textwrap.dedent('''Saving debug log to /var/log/letsencrypt/letsencrypt.log + Plugins selected: Authenticator standalone, Installer None + Starting new HTTPS connection (1): acme-v02.api.letsencrypt.org + Obtaining a new certificate + Resetting dropped connection: acme-v02.api.letsencrypt.org + '''), + 'retcode': 0, + } + result_new_cert = { + "comment": "Certificate test obtained", + "not_after": datetime.datetime.fromtimestamp(valid_timestamp).isoformat(), + "changes": { + "mode": "0640" + }, + "result": True + } + + cmd_no_renew = { + 'stdout': textwrap.dedent(''' + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + Certificate not yet due for renewal; no action taken. + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + '''), + 'stderr': textwrap.dedent('''Saving debug log to /var/log/letsencrypt/letsencrypt.log + Plugins selected: Authenticator standalone, Installer None + Starting new HTTPS connection (1): acme-v02.api.letsencrypt.org + Cert not yet due for renewal + Keeping the existing certificate + '''), + 'retcode': 0 + } + result_no_renew = { + "comment": "Certificate /etc/letsencrypt/live/test/cert.pem unchanged", + "not_after": datetime.datetime.fromtimestamp(valid_timestamp).isoformat(), + "changes": {}, + "result": True + } + result_renew = { + "comment": "Certificate test renewed", + "not_after": datetime.datetime.fromtimestamp(expired_timestamp).isoformat(), + "changes": {}, + "result": True + } + + # Test fetching new certificate + with patch('salt.modules.acme.LEA', 'certbot'), \ + patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run_all': MagicMock(return_value=cmd_new_cert), + 'file.file_exists': MagicMock(return_value=False), + 'tls.cert_info': MagicMock(return_value={'not_after': valid_timestamp}), + 'file.check_perms': MagicMock( + side_effect=lambda a, x, b, c, d, follow_symlinks: ( + salt.utils.dictupdate.set_dict_key_value(x, 'changes:mode', '0640'), + None + ) + ) + }): + self.assertEqual(acme.cert('test'), result_new_cert) + # Test not renewing a valid certificate + with patch('salt.modules.acme.LEA', 'certbot'), \ + patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run_all': MagicMock(return_value=cmd_no_renew), + 'file.file_exists': MagicMock(return_value=True), + 'tls.cert_info': MagicMock(return_value={'not_after': valid_timestamp}), + 'file.check_perms': MagicMock( + side_effect=lambda a, x, b, c, d, follow_symlinks: ( + salt.utils.dictupdate.set_dict_key_value(x, 'result', True), + None + ) + ) + }): + self.assertEqual(acme.cert('test'), result_no_renew) + # Test renewing an expired certificate + with patch('salt.modules.acme.LEA', 'certbot'), \ + patch.dict(acme.__salt__, { # pylint: disable=no-member + 'cmd.run_all': MagicMock(return_value=cmd_new_cert), + 'file.file_exists': MagicMock(return_value=True), + 'tls.cert_info': MagicMock(return_value={'not_after': expired_timestamp}), + 'file.check_perms': MagicMock( + side_effect=lambda a, x, b, c, d, follow_symlinks: ( + salt.utils.dictupdate.set_dict_key_value(x, 'result', True), + None + ) + ) + }): + self.assertEqual(acme.cert('test'), result_renew) diff --git a/tests/unit/states/test_acme.py b/tests/unit/states/test_acme.py new file mode 100644 index 000000000000..c5deaa063ace --- /dev/null +++ b/tests/unit/states/test_acme.py @@ -0,0 +1,128 @@ +# -*- coding: utf-8 -*- +''' +:codeauthor: Herbert Buurman +''' + +# Import future libs +from __future__ import absolute_import, print_function, unicode_literals + +# Import Salt Testing libs +from tests.support.mixins import LoaderModuleMockMixin +from tests.support.unit import skipIf, TestCase +from tests.support.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch + +# Import Salt Module +import salt.states.acme as acme + + +@skipIf(NO_MOCK, NO_MOCK_REASON) +class AcmeTestCase(TestCase, LoaderModuleMockMixin): + ''' + Test cases for salt.modules.acme + ''' + + def setup_loader_modules(self): + return {acme: {'__opts__': {'test': False}}} + + def test_cert_no_changes_t(self): + ''' + Test cert state with no needed changes. (test=True) + ''' + # With test=True + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=False), + }), \ + patch.dict(acme.__opts__, {'test': True}): # pylint: disable=no-member + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Certificate test exists and does not need renewal.'], + 'changes': {}, + }) + + def test_cert_no_changes(self): + ''' + Test cert state with no needed changes. (test=False) + ''' + # With test=False + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=False), + }): + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Certificate test exists and does not need renewal.'], + 'changes': {}, + }) + + def test_cert_fresh_certificate_t(self): + ''' + Test cert state fetching a new certificate. (test=True) + ''' + # With test=True + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=False), + # 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + 'acme.info': MagicMock(return_value={'foo': 'bar'}), + }), \ + patch.dict(acme.__opts__, {'test': True}): # pylint: disable=no-member + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': None, + 'comment': ['Certificate test would have been obtained.'], + 'changes': {'old': 'current certificate', 'new': 'new certificate'}, + }) + + def test_cert_fresh_certificate(self): + ''' + Test cert state fetching a new certificate. (test=False) + ''' + # With test=False + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=False), + 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + 'acme.info': MagicMock(return_value={'foo': 'bar'}), + }): + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Mockery'], + 'changes': {'new': {'foo': 'bar'}}, + }) + + def test_cert_renew_certificate_t(self): + ''' + Test cert state renewing a certificate. (test=True) + ''' + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=True), + 'acme.info': MagicMock(side_effect=[{'name': 'old cert'}, {'name': 'new cert'}]), + 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + }), \ + patch.dict(acme.__opts__, {'test': True}): # pylint: disable=no-member + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': None, + 'comment': ['Certificate test would have been renewed.'], + 'changes': {'old': 'current certificate', 'new': 'new certificate'}, + }) + + def test_cert_renew_certificate(self): + ''' + Test cert state renewing a certificate. (test=False) + ''' + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'acme.has': MagicMock(return_value=True), + 'acme.needs_renewal': MagicMock(return_value=True), + 'acme.info': MagicMock(side_effect=[{'name': 'old cert'}, {'name': 'new cert'}]), + 'acme.cert': MagicMock(return_value={'result': True, 'comment': 'Mockery'}), + }): + self.assertEqual(acme.cert('test'), { + 'name': 'test', + 'result': True, + 'comment': ['Mockery'], + 'changes': {'old': {'name': 'old cert'}, 'new': {'name': 'new cert'}}, + }) From a9f4a4fd8329a60e6bf785b6639c690ecceca3e0 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Wed, 5 Jun 2019 11:41:09 +0200 Subject: [PATCH 4/7] salt/modules/acme.py: Changed "info" to always return a dict. Added x509.read_certificate as alternative method of getting certificate information if tls.cert_info is not available. --- salt/modules/acme.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/salt/modules/acme.py b/salt/modules/acme.py index 8136c7e1cd76..c79fc9b481cf 100644 --- a/salt/modules/acme.py +++ b/salt/modules/acme.py @@ -86,7 +86,6 @@ def _expires(name): strptime_sux_cmd = 'date --date="$({0} | cut -d= -f2)" +%s'.format(openssl_cmd) expiry = float(__salt__['cmd.shell'](strptime_sux_cmd, output_loglevel='quiet')) # expiry = datetime.datetime.strptime(expiry.split('=', 1)[-1], '%b %e %H:%M:%S %Y %Z') - return datetime.datetime.fromtimestamp(expiry) @@ -285,6 +284,7 @@ def info(name): Will output tls.cert_info if that's available, or OpenSSL text if not :param str name: CommonName of certificate + :return dict CLI example: @@ -293,19 +293,22 @@ def info(name): salt 'gitlab.example.com' acme.info dev.example.com ''' if not has(name): - return None + return {} cert_file = _cert_file(name, 'cert') - # Use the salt module if available + # Use the tls salt module if available if 'tls.cert_info' in __salt__: cert_info = __salt__['tls.cert_info'](cert_file) # Strip out the extensions object contents; # these trip over our poor state output # and they serve no real purpose here anyway cert_info['extensions'] = cert_info['extensions'].keys() - return cert_info - # Cobble it together using the openssl binary - openssl_cmd = 'openssl x509 -in {0} -noout -text'.format(cert_file) - return __salt__['cmd.run'](openssl_cmd, output_loglevel='quiet') + elif 'x509.read_certificate' in __salt__: + cert_info = __salt__['x509.read_certificate'](cert_file) + else: + # Cobble it together using the openssl binary + openssl_cmd = 'openssl x509 -in {0} -noout -text'.format(cert_file) + cert_info = {'text': __salt__['cmd.run'](openssl_cmd, output_loglevel='quiet')} + return cert_info def expires(name): From ed24491d2a6992d6734146e7dec07be29b43e29f Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Wed, 5 Jun 2019 11:41:23 +0200 Subject: [PATCH 5/7] salt/states/acme.py: Removed unused import --- salt/states/acme.py | 1 - 1 file changed, 1 deletion(-) diff --git a/salt/states/acme.py b/salt/states/acme.py index 3ae7a508ba93..b4c0101bd824 100644 --- a/salt/states/acme.py +++ b/salt/states/acme.py @@ -31,7 +31,6 @@ # Import salt libs import salt.utils.dictdiffer -import salt.ext.six as six log = logging.getLogger(__name__) From 5e581b9e3c8beeff0566fc1d8086ade086ec9e6f Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Wed, 5 Jun 2019 11:42:48 +0200 Subject: [PATCH 6/7] tests/unit/modules/test_acme.py: Added test for salt.modules.acme.info using x509.read_certificates. Fixed incorrect data placement in textwrap.dedent. Updated test for acme.info to expect dict when using openssl cli. --- tests/unit/modules/test_acme.py | 48 +++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/unit/modules/test_acme.py b/tests/unit/modules/test_acme.py index 4d1d4b10c057..f4d7743ef3e0 100644 --- a/tests/unit/modules/test_acme.py +++ b/tests/unit/modules/test_acme.py @@ -88,7 +88,7 @@ def test_info(self): ''' Test certificate information retrieval. ''' - certinfo_result = { + certinfo_tls_result = { "not_after": 1559471377, "signature_algorithm": "sha256WithRSAEncryption", "extensions": {}, @@ -99,16 +99,46 @@ def test_info(self): "not_before": 1559557777, "subject": {}, } + certinfo_x509_result = { + "Not After": "2019-06-02 10:29:37", + "Subject Hash": "54:3B:6C:A4", + "Serial Number": "59:AB:CB:A0:FB:90:E8:4B", + "SHA1 Finger Print": "F1:8D:F3:26:1B:D3:88:32:CD:B6:FA:3B:85:58:DA:C7:6F:62:BE:7E", + "SHA-256 Finger Print": ("FB:A4:5F:71:D6:5D:6C:B6:1D:2C:FD:91:09:2C:1C:52:" + "3C:EC:B6:4D:1A:95:65:37:04:D0:E2:5E:C7:64:0C:9C"), + "MD5 Finger Print": "95:B5:96:9B:42:A5:9E:20:78:FD:99:09:4B:21:1E:97", + "Version": 3, + "Key Size": 2048, + "Public Key": ("-----BEGIN PUBLIC KEY-----\n" + "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsVO2vwQPKU92PSBnuGid\n" + "k8t6KWVE2jEBM10u7CgqQmD/JCnYflEHAo1nOsD7wxdhBrxhf5Qs+pEX1HOsh8VA\n" + "HDTim0iE8nQVJ0Iuen2SrwaWMhwKmZTSJRYMgd46oCMi2RdlCvcgF2Hw6RTwF7FT\n" + "hnksc4HBT91XddnP32N558tOT3YejafQNvClz5WcR+E0JzqGrV/+wfe3o+j/q5eK\n" + "UowttWazeSMvuROtqj/fEk0rop4D14pgzZqWi30tjwhJNl6fSPFWBrLEHGNyDJ+O\n" + "zfov0B2MRLJibH7GMkOCwsP2g1lVOReqcml+ju6zAKW8nHBTRg0iXB18Ifxef57Y\n" + "AQIDAQAB\n" + "-----END PUBLIC KEY-----\n"), + "Issuer": {}, + "Issuer Hash": "54:3B:6C:A4", + "Not Before": "2019-06-03 10:29:37", + "Subject": {} + } + with patch.dict(acme.__salt__, { # pylint: disable=no-member - 'tls.cert_info': MagicMock(return_value=certinfo_result), - 'file.file_exists': MagicMock(return_value=True) + 'file.file_exists': MagicMock(return_value=True), + 'tls.cert_info': MagicMock(return_value=certinfo_tls_result), }): - self.assertEqual(acme.info('test'), certinfo_result) + self.assertEqual(acme.info('test'), certinfo_tls_result) with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'file.file_exists': MagicMock(return_value=True), + 'x509.read_certificate': MagicMock(return_value=certinfo_x509_result), + }): + self.assertEqual(acme.info('test'), certinfo_x509_result) + with patch.dict(acme.__salt__, { # pylint: disable=no-member + 'file.file_exists': MagicMock(return_value=True), 'cmd.run': MagicMock(return_value='foo'), - 'file.file_exists': MagicMock(return_value=True) }): - self.assertEqual(acme.info('test'), 'foo') + self.assertEqual(acme.info('test'), {'text': 'foo'}) def test_cert(self): ''' @@ -119,7 +149,8 @@ def test_cert(self): expired_timestamp = (datetime.datetime.now() - datetime.timedelta(days=3) - datetime.datetime(1970, 1, 1, 0, 0, 0, 0)).total_seconds() cmd_new_cert = { - 'stdout': textwrap.dedent('''IMPORTANT NOTES: + 'stdout': textwrap.dedent(''' + IMPORTANT NOTES: - Congratulations! Your certificate and chain have been saved at: /etc/letsencrypt/live/test/fullchain.pem Your key file has been saved at: @@ -133,7 +164,8 @@ def test_cert(self): Donating to ISRG / Let's Encrypt: https://letsencrypt.org/donate Donating to EFF: https://eff.org/donate-le '''), - 'stderr': textwrap.dedent('''Saving debug log to /var/log/letsencrypt/letsencrypt.log + 'stderr': textwrap.dedent(''' + Saving debug log to /var/log/letsencrypt/letsencrypt.log Plugins selected: Authenticator standalone, Installer None Starting new HTTPS connection (1): acme-v02.api.letsencrypt.org Obtaining a new certificate From b775e8e87158a99f6b1653385b410f21995fe140 Mon Sep 17 00:00:00 2001 From: Herbert Buurman Date: Fri, 26 Jul 2019 10:43:21 +0200 Subject: [PATCH 7/7] Optimized convoluted window check in "needs_renewal". Improved/fixed function docstrings. --- salt/modules/acme.py | 64 +++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/salt/modules/acme.py b/salt/modules/acme.py index c79fc9b481cf..f5422cf3246f 100644 --- a/salt/modules/acme.py +++ b/salt/modules/acme.py @@ -73,7 +73,8 @@ def _expires(name): ''' Return the expiry date of a cert - :return datetime object of expiry date + :rtype: datetime + :return: Expiry date ''' cert_file = _cert_file(name, 'cert') # Use the salt module if available @@ -95,7 +96,8 @@ def _renew_by(name, window=None): :param str name: Common Name of the certificate (DNS name of certificate) :param int window: days before expiry date to renew - :return datetime object of first renewal date + :rtype: datetime + :return: First renewal date ''' expiry = _expires(name) if window is not None: @@ -131,8 +133,10 @@ def cert(name, :param aliases: subjectAltNames (Additional DNS names on certificate) :param email: e-mail address for interaction with ACME provider :param webroot: True or a full path to use to use webroot. Otherwise use standalone mode - :param test_cert: Request a certificate from the Happy Hacker Fake CA (mutually exclusive with 'server') - :param renew: True/'force' to force a renewal, or a window of renewal before expiry in days + :param test_cert: Request a certificate from the Happy Hacker Fake CA (mutually + exclusive with 'server') + :param renew: True/'force' to force a renewal, or a window of renewal before + expiry in days :param keysize: RSA key bits :param server: API endpoint to talk to :param owner: owner of the private key file @@ -140,22 +144,26 @@ def cert(name, :param mode: mode of the private key file :param certname: Name of the certificate to save :param preferred_challenges: A sorted, comma delimited list of the preferred - challenge to use during authorization with the - most preferred challenge listed first. + challenge to use during authorization with the most preferred challenge + listed first. :param tls_sni_01_port: Port used during tls-sni-01 challenge. This only affects - the port Certbot listens on. A conforming ACME server - will still attempt to connect on port 443. + the port Certbot listens on. A conforming ACME server will still attempt + to connect on port 443. :param tls_sni_01_address: The address the server listens to during tls-sni-01 - challenge. + challenge. :param http_01_port: Port used in the http-01 challenge. This only affects - the port Certbot listens on. A conforming ACME server - will still attempt to connect on port 80. + the port Certbot listens on. A conforming ACME server will still attempt + to connect on port 80. :param https_01_address: The address the server listens to during http-01 challenge. - :param dns_plugin: Name of a DNS plugin to use (currently only 'cloudflare' or 'digitalocean') - :param dns_plugin_credentials: Path to the credentials file if required by the specified DNS plugin - :param dns_plugin_propagate_seconds: Number of seconds to wait for DNS propogations before - asking ACME servers to verify the DNS record. (default 10) - :return: dict with 'result' True/False/None, 'comment' and certificate's expiry date ('not_after') + :param dns_plugin: Name of a DNS plugin to use (currently only 'cloudflare' + or 'digitalocean') + :param dns_plugin_credentials: Path to the credentials file if required by + the specified DNS plugin + :param dns_plugin_propagate_seconds: Number of seconds to wait for DNS propogations + before asking ACME servers to verify the DNS record. (default 10) + :rtype: dict + :return: Dictionary with 'result' True/False/None, 'comment' and certificate's + expiry date ('not_after') CLI example: @@ -280,11 +288,12 @@ def info(name): ''' Return information about a certificate - .. note:: - Will output tls.cert_info if that's available, or OpenSSL text if not - :param str name: CommonName of certificate - :return dict + :rtype: dict + :return: Dictionary with information about the certificate. + If neither the ``tls`` nor the ``x509`` module can be used to determine + the certificate information, the information will be retrieved as one + big text block under the key ``text`` using the openssl cli. CLI example: @@ -316,6 +325,8 @@ def expires(name): The expiry date of a certificate in ISO format :param str name: CommonName of certificate + :rtype: str + :return: Expiry date in ISO format. CLI example: @@ -331,6 +342,7 @@ def has(name): Test if a certificate is in the Let's Encrypt Live directory :param str name: CommonName of certificate + :rtype: bool Code example: @@ -348,6 +360,8 @@ def renew_by(name, window=None): :param str name: CommonName of certificate :param int window: number of days before expiry when renewal should take place + :rtype: str + :return: Date of certificate renewal in ISO format. ''' return _renew_by(name, window).isoformat() @@ -358,6 +372,8 @@ def needs_renewal(name, window=None): :param str name: CommonName of certificate :param bool/str/int window: Window in days to renew earlier or True/force to just return True + :rtype: bool + :return: Whether or not the certificate needs to be renewed. Code example: @@ -368,16 +384,14 @@ def needs_renewal(name, window=None): else: log.info('Your certificate is still good') ''' - if window is not None and window in ('force', 'Force', True): - return True - if window: + if str(window).lower in ('force', 'true'): + return True if not (isinstance(window, int) or (hasattr(window, 'isdigit') and window.isdigit())): raise SaltInvocationError( 'The argument "window", if provided, must be one of the following : ' 'True (boolean), "force" or "Force" (str) or a numerical value in days.' ) - else: - window = int(window) + window = int(window) return _renew_by(name, window) <= datetime.datetime.today()