From e9e49d17d6fbd986d23a71f1bb415dca30cb3011 Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sun, 15 Dec 2019 12:50:32 -0600 Subject: [PATCH 01/10] Rewrite x509.certificate_managed to be easier to use The function now displays clearer error messages when a problem occurs and informative messages when comparing an existing certificate. test=True is now supported. It fixes #52180, #39608, #41858 and others: * Error messages from the x509 module calls are written directly to the certificate file - fixed, the certificate file is only created when the x509 module calls succeed. * Certificates are created when no changes are required - fixed, the comparison logic has been updated. The `managed_private_key` option has been removed due to the added complexity. The functionality can easily be replicated with an additional call to `x509.private_key_managed`. According to the comment at https://github.com/saltstack/salt/issues/39608#issuecomment-342346894 `managed_private_key` has not worked since at least v2016.11.2. --- salt/states/x509.py | 395 ++++++++++++++++++++++++-------------------- 1 file changed, 215 insertions(+), 180 deletions(-) diff --git a/salt/states/x509.py b/salt/states/x509.py index 101cd564b143..d0b71d9b6009 100644 --- a/salt/states/x509.py +++ b/salt/states/x509.py @@ -63,6 +63,12 @@ /etc/pki/issued_certs: file.directory + /etc/pki/ca.crt: + x509.private_key_managed: + - name: /etc/pki/ca.key + - bits: 4096 + - backup: True + /etc/pki/ca.crt: x509.certificate_managed: - signing_private_key: /etc/pki/ca.key @@ -77,10 +83,6 @@ - days_valid: 3650 - days_remaining: 0 - backup: True - - managed_private_key: - name: /etc/pki/ca.key - bits: 4096 - backup: True - require: - file: /etc/pki @@ -139,6 +141,12 @@ .. code-block:: yaml + /etc/pki/www.crt: + x509.private_key_managed: + - name: /etc/pki/www.key + - bits: 4096 + - backup: True + /etc/pki/www.crt: x509.certificate_managed: - ca_server: ca @@ -147,20 +155,14 @@ - CN: www.example.com - days_remaining: 30 - backup: True - - managed_private_key: - name: /etc/pki/www.key - bits: 4096 - backup: True - """ # Import Python Libs -from __future__ import absolute_import, print_function, unicode_literals - -import copy +from __future__ import absolute_import, unicode_literals, print_function import datetime import os import re +import copy # Import Salt Libs import salt.exceptions @@ -272,7 +274,8 @@ def private_key_managed( new: Always create a new key. Defaults to ``False``. - Combining new with :mod:`prereq `, or when used as part of a `managed_private_key` can allow key rotation whenever a new certificate is generated. + Combining new with :mod:`prereq ` + can allow key rotation whenever a new certificate is generated. overwrite: Overwrite an existing private key if the provided passphrase cannot decrypt it. @@ -370,9 +373,138 @@ def csr_managed(name, **kwargs): return ret -def certificate_managed( - name, days_remaining=90, managed_private_key=None, append_certs=None, **kwargs -): +def _certificate_info_matches(cert_info, required_cert_info, check_serial=False): + """ + Return true if the provided certificate information matches the + required certificate information, i.e. it has the required common + name, subject alt name, organization, etc. + + cert_info should be a dict as returned by x509.read_certificate. + required_cert_info should be a dict as returned by x509.create_certificate with testrun=True. + """ + # don't modify the incoming dicts + cert_info = copy.deepcopy(cert_info) + required_cert_info = copy.deepcopy(required_cert_info) + + ignored_keys = [ + "Not Before", + "Not After", + "MD5 Finger Print", + "SHA1 Finger Print", + "SHA-256 Finger Print", + # The integrity of the issuer is checked elsewhere + "Issuer Public Key", + ] + for key in ignored_keys: + cert_info.pop(key, None) + required_cert_info.pop(key, None) + + if not check_serial: + cert_info.pop("Serial Number", None) + required_cert_info.pop("Serial Number", None) + try: + cert_info["X509v3 Extensions"]["authorityKeyIdentifier"] = re.sub( + r"serial:([0-9A-F]{2}:)*[0-9A-F]{2}", + "serial:--", + cert_info["X509v3 Extensions"]["authorityKeyIdentifier"], + ) + required_cert_info["X509v3 Extensions"]["authorityKeyIdentifier"] = re.sub( + r"serial:([0-9A-F]{2}:)*[0-9A-F]{2}", + "serial:--", + required_cert_info["X509v3 Extensions"]["authorityKeyIdentifier"], + ) + except KeyError: + pass + + diff = [] + for k, v in six.iteritems(required_cert_info): + try: + if v != cert_info[k]: + diff.append(k) + except KeyError: + diff.append(k) + + return len(diff) == 0, diff + + +def _certificate_days_remaining(cert_info): + """ + Get the days remaining on a certificate, defaulting to 0 if an error occurs. + """ + try: + expiry = cert_info["Not After"] + return ( + datetime.datetime.strptime(expiry, "%Y-%m-%d %H:%M:%S") + - datetime.datetime.now() + ).days + except KeyError: + return 0 + + +def _certificate_is_valid(name, days_remaining, append_certs, **cert_spec): + """ + Return True if the given certificate file exists, is a certificate, matches the given specification, and has the required days remaining. + + If False, also provide a message explaining why. + """ + if not os.path.isfile(name): + return False, "{0} does not exist".format(name), {} + + try: + cert_info = __salt__["x509.read_certificate"](certificate=name) + required_cert_info = __salt__["x509.create_certificate"]( + testrun=True, **cert_spec + ) + if not isinstance(required_cert_info, dict): + raise salt.exceptions.SaltInvocationError( + "Unable to create new certificate: x509 module error: {0}".format( + required_cert_info + ) + ) + + try: + issuer_public_key = required_cert_info["Issuer Public Key"] + # Verify the certificate has been signed by the ca_server or private_signing_key + if not __salt__["x509.verify_signature"](name, issuer_public_key): + errmsg = ( + "Certificate is not signed by private_signing_key" + if "signing_private_key" in cert_spec + else "Certificate is not signed by the requested issuer" + ) + return False, errmsg, cert_info + except KeyError: + return ( + False, + "New certificate does not include signing information", + cert_info, + ) + + matches, diff = _certificate_info_matches( + cert_info, required_cert_info, check_serial="serial_number" in cert_spec + ) + if not matches: + return ( + False, + "Certificate properties are different: {0}".format(", ".join(diff)), + cert_info, + ) + + actual_days_remaining = _certificate_days_remaining(cert_info) + if days_remaining != 0 and actual_days_remaining < days_remaining: + return ( + False, + "Certificate needs renewal: {0} days remaining but it needs to be at least {1}".format( + actual_days_remaining, days_remaining + ), + cert_info, + ) + + return True, "", cert_info + except salt.exceptions.SaltInvocationError as e: + return False, "{0} is not a valid certificate: {1}".format(name, str(e)), {} + + +def certificate_managed(name, days_remaining=90, append_certs=None, **kwargs): """ Manage a Certificate @@ -383,13 +515,6 @@ def certificate_managed( The minimum number of days remaining when the certificate should be recreated. A value of 0 disables automatic renewal. - managed_private_key - Manages the private key corresponding to the certificate. All of the - arguments supported by :py:func:`x509.private_key_managed - ` are supported. If `name` is not - specified or is the same as the name of the certificate, the private - key and certificate will be written together in the same file. - append_certs: A list of certificates to be appended to the managed file. @@ -434,173 +559,83 @@ def certificate_managed( if "path" in kwargs: name = kwargs.pop("path") - file_args, kwargs = _get_file_args(name, **kwargs) - - rotate_private_key = False - new_private_key = False - if managed_private_key: - private_key_args = { - "name": name, - "new": False, - "overwrite": False, - "bits": 2048, - "passphrase": None, - "cipher": "aes_128_cbc", - "verbose": True, - } - private_key_args.update(managed_private_key) - kwargs["public_key_passphrase"] = private_key_args["passphrase"] - - if private_key_args["new"]: - rotate_private_key = True - private_key_args["new"] = False - - if _check_private_key( - private_key_args["name"], - bits=private_key_args["bits"], - passphrase=private_key_args["passphrase"], - new=private_key_args["new"], - overwrite=private_key_args["overwrite"], - ): - private_key = __salt__["x509.get_pem_entry"]( - private_key_args["name"], pem_type="RSA PRIVATE KEY" - ) - else: - new_private_key = True - private_key = __salt__["x509.create_private_key"]( - text=True, - bits=private_key_args["bits"], - passphrase=private_key_args["passphrase"], - cipher=private_key_args["cipher"], - verbose=private_key_args["verbose"], - ) - - kwargs["public_key"] = private_key - - current_days_remaining = 0 - current_comp = {} - - if os.path.isfile(name): - try: - current = __salt__["x509.read_certificate"](certificate=name) - current_comp = copy.deepcopy(current) - if "serial_number" not in kwargs: - current_comp.pop("Serial Number") - if "signing_cert" not in kwargs: - try: - current_comp["X509v3 Extensions"][ - "authorityKeyIdentifier" - ] = re.sub( - r"serial:([0-9A-F]{2}:)*[0-9A-F]{2}", - "serial:--", - current_comp["X509v3 Extensions"]["authorityKeyIdentifier"], - ) - except KeyError: - pass - current_comp.pop("Not Before") - current_comp.pop("MD5 Finger Print") - current_comp.pop("SHA1 Finger Print") - current_comp.pop("SHA-256 Finger Print") - current_notafter = current_comp.pop("Not After") - current_days_remaining = ( - datetime.datetime.strptime(current_notafter, "%Y-%m-%d %H:%M:%S") - - datetime.datetime.now() - ).days - if days_remaining == 0: - days_remaining = current_days_remaining - 1 - except salt.exceptions.SaltInvocationError: - current = "{0} is not a valid Certificate.".format(name) - else: - current = "{0} does not exist.".format(name) - if "ca_server" in kwargs and "signing_policy" not in kwargs: raise salt.exceptions.SaltInvocationError( "signing_policy must be specified if ca_server is." ) - new = __salt__["x509.create_certificate"](testrun=True, **kwargs) - - if isinstance(new, dict): - new_comp = copy.deepcopy(new) - new.pop("Issuer Public Key") - if "serial_number" not in kwargs: - new_comp.pop("Serial Number") - if "signing_cert" not in kwargs: - try: - new_comp["X509v3 Extensions"]["authorityKeyIdentifier"] = re.sub( - r"serial:([0-9A-F]{2}:)*[0-9A-F]{2}", - "serial:--", - new_comp["X509v3 Extensions"]["authorityKeyIdentifier"], - ) - except KeyError: - pass - new_comp.pop("Not Before") - new_comp.pop("Not After") - new_comp.pop("MD5 Finger Print") - new_comp.pop("SHA1 Finger Print") - new_comp.pop("SHA-256 Finger Print") - new_issuer_public_key = new_comp.pop("Issuer Public Key") - else: - new_comp = new + if "public_key" not in kwargs and "signing_private_key" not in kwargs: + raise salt.exceptions.SaltInvocationError( + "public_key or signing_private_key must be specified." + ) - new_certificate = False - if ( - current_comp == new_comp - and current_days_remaining > days_remaining - and __salt__["x509.verify_signature"](name, new_issuer_public_key) - ): - certificate = __salt__["x509.get_pem_entry"](name, pem_type="CERTIFICATE") - else: - if rotate_private_key and not new_private_key: - new_private_key = True - private_key = __salt__["x509.create_private_key"]( - text=True, - bits=private_key_args["bits"], - verbose=private_key_args["verbose"], - ) - kwargs["public_key"] = private_key - new_certificate = True - certificate = __salt__["x509.create_certificate"](text=True, **kwargs) - - file_args["contents"] = "" - private_ret = {} - if managed_private_key: - if private_key_args["name"] == name: - file_args["contents"] = private_key - else: - private_file_args = copy.deepcopy(file_args) - unique_private_file_args, _ = _get_file_args(**private_key_args) - private_file_args.update(unique_private_file_args) - private_file_args["contents"] = private_key - private_ret = __states__["file.managed"](**private_file_args) - if not private_ret["result"]: - return private_ret + ret = {"name": name, "result": False, "changes": {}, "comment": ""} - file_args["contents"] += salt.utils.stringutils.to_str(certificate) + is_valid, invalid_reason, current_cert_info = _certificate_is_valid( + name, days_remaining, append_certs, **kwargs + ) - if not append_certs: - append_certs = [] - for append_cert in append_certs: - file_args["contents"] += __salt__["x509.get_pem_entry"]( - append_cert, pem_type="CERTIFICATE" - ) + if is_valid: + ret["result"] = True + ret["comment"] = "Certificate {0} is valid and up to date".format(name) + return ret + + if __opts__["test"]: + ret["result"] = None + ret["comment"] = "Certificate {0} will be created".format(name) + ret["changes"]["Status"] = { + "Old": invalid_reason, + "New": "Certificate will be valid and up to date", + } + return ret - file_args["show_changes"] = False - ret = __states__["file.managed"](**file_args) + contents = __salt__["x509.create_certificate"](text=True, **kwargs) + # Check the module actually returned a cert and not an error message as a string + try: + __salt__["x509.read_certificate"](contents) + except salt.exceptions.SaltInvocationError as e: + ret["result"] = False + ret[ + "comment" + ] = "An error occurred creating the certificate {0}. The result returned from x509.create_certificate is not a valid PEM file:\n{1}".format( + name, str(e) + ) + return ret - if ret["changes"]: - ret["changes"] = {"Certificate": ret["changes"]} - else: - ret["changes"] = {} - if private_ret and private_ret["changes"]: - ret["changes"]["Private Key"] = private_ret["changes"] - if new_private_key: - ret["changes"]["Private Key"] = "New private key generated" - if new_certificate: - ret["changes"]["Certificate"] = { - "Old": current, - "New": __salt__["x509.read_certificate"](certificate=certificate), - } + if not append_certs: + append_certs = [] + for append_file in append_certs: + try: + append_file_contents = __salt__["x509.get_pem_entry"]( + append_file, pem_type="CERTIFICATE" + ) + contents += append_file_contents + except salt.exceptions.SaltInvocationError as e: + ret["result"] = False + ret[ + "comment" + ] = "{0} is not a valid certificate file, cannot append it to the certificate {1}.\nThe error returned by the x509 module was:\n{2}".format( + append_file, name, str(e) + ) + return ret + + file_args, extra_args = _get_file_args(name, **kwargs) + file_args["contents"] = contents + file_ret = __states__["file.managed"](**file_args) + + if file_ret["changes"]: + ret["changes"] = {"File": file_ret["changes"]} + + ret["changes"]["Certificate"] = { + "Old": current_cert_info, + "New": __salt__["x509.read_certificate"](certificate=name), + } + ret["changes"]["Status"] = { + "Old": invalid_reason, + "New": "Certificate is valid and up to date", + } + ret["comment"] = "Certificate {0} is valid and up to date".format(name) + ret["result"] = True return ret From 1456cb334c742135d4ca0df55945981d12e589ba Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 8 Feb 2020 14:18:14 +0100 Subject: [PATCH 02/10] Add integration test for a self-signed certificate --- .../files/file/base/x509/self_signed.sls | 15 +++++++ tests/integration/states/test_x509.py | 44 ++++++++++++++++--- 2 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/integration/files/file/base/x509/self_signed.sls diff --git a/tests/integration/files/file/base/x509/self_signed.sls b/tests/integration/files/file/base/x509/self_signed.sls new file mode 100644 index 000000000000..90fb5282dabe --- /dev/null +++ b/tests/integration/files/file/base/x509/self_signed.sls @@ -0,0 +1,15 @@ +{% set tmp_dir = pillar['tmp_dir'] %} + +private_key: + x509.private_key_managed: + - name: {{ tmp_dir }}/self.key + +self_signed_cert: + x509.certificate_managed: + - name: {{ tmp_dir }}/self.crt + - signing_private_key: {{ tmp_dir }}/self.key + - CN: localhost + - days_valid: 30 + - days_remaining: 0 + - require: + - x509: private_key diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index 0a720f8575a9..c5ee94bed683 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -1,17 +1,17 @@ # -*- coding: utf-8 -*- from __future__ import absolute_import, unicode_literals - -import logging import os -import textwrap +import logging import salt.utils.files from salt.ext import six -from tests.support.case import ModuleCase +import textwrap + from tests.support.helpers import with_tempfile +from tests.support.case import ModuleCase +from tests.support.unit import skipIf from tests.support.mixins import SaltReturnAssertsMixin from tests.support.runtests import RUNTIME_VARS -from tests.support.unit import skipIf try: import M2Crypto # pylint: disable=W0611 @@ -119,3 +119,37 @@ def test_cert_signing(self): assert "changes" in ret[key] assert "Certificate" in ret[key]["changes"] assert "New" in ret[key]["changes"]["Certificate"] + + def test_self_signed_cert(self): + """ + Self-signed certificate, no CA. + Run the state twice to confirm the cert is only created once and its contents don't change. + """ + first_run = self.run_function( + "state.apply", ["x509.self_signed"], pillar={"tmp_dir": RUNTIME_VARS.TMP} + ) + key = "x509_|-self_signed_cert_|-{}/self.crt_|-certificate_managed".format( + RUNTIME_VARS.TMP + ) + self.assertIn("New", first_run[key]["changes"]["Certificate"]) + self.assertEqual( + "Certificate is valid and up to date", + first_run[key]["changes"]["Status"]["New"], + ) + cert_path = first_run[key]["name"] + self.assertTrue(os.path.exists(cert_path), "Certificate was not created.") + + with salt.utils.files.fopen(cert_path, "r") as first_cert: + cert_contents = first_cert.read() + + second_run = self.run_function( + "state.apply", ["x509.self_signed"], pillar={"tmp_dir": RUNTIME_VARS.TMP} + ) + key = "x509_|-self_signed_cert_|-{}/self.crt_|-certificate_managed".format( + RUNTIME_VARS.TMP + ) + self.assertEqual({}, second_run[key]["changes"]) + with salt.utils.files.fopen(cert_path, "r") as second_cert: + self.assertEqual( + cert_contents, second_cert.read(), "Certificate contents have changed." + ) From 1c0441d2ce47043e1195c2ba6e353cb018635a52 Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 8 Feb 2020 15:38:08 +0100 Subject: [PATCH 03/10] Test self signed certificates are renewed correctly --- .../file/base/x509/self_signed_expiry.sls | 18 +++++ tests/integration/states/test_x509.py | 65 ++++++++++++++++++- 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 tests/integration/files/file/base/x509/self_signed_expiry.sls diff --git a/tests/integration/files/file/base/x509/self_signed_expiry.sls b/tests/integration/files/file/base/x509/self_signed_expiry.sls new file mode 100644 index 000000000000..89720fe47b9b --- /dev/null +++ b/tests/integration/files/file/base/x509/self_signed_expiry.sls @@ -0,0 +1,18 @@ +{% set keyfile = pillar['keyfile'] %} +{% set crtfile = pillar['crtfile'] %} +{% set days_valid = pillar['days_valid'] %} +{% set days_remaining = pillar['days_remaining'] %} + +private_key: + x509.private_key_managed: + - name: {{ keyfile }} + +self_signed_cert: + x509.certificate_managed: + - name: {{ crtfile }} + - signing_private_key: {{ keyfile }} + - CN: localhost + - days_valid: {{ days_valid }} + - days_remaining: {{ days_remaining }} + - require: + - x509: private_key diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index c5ee94bed683..1ceb90a638d3 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -2,6 +2,7 @@ from __future__ import absolute_import, unicode_literals import os import logging +import datetime import salt.utils.files from salt.ext import six @@ -151,5 +152,67 @@ def test_self_signed_cert(self): self.assertEqual({}, second_run[key]["changes"]) with salt.utils.files.fopen(cert_path, "r") as second_cert: self.assertEqual( - cert_contents, second_cert.read(), "Certificate contents have changed." + cert_contents, + second_cert.read(), + "Certificate contents should not have changed.", + ) + + @with_tempfile(suffix=".crt", create=False) + @with_tempfile(suffix=".key", create=False) + def test_old_self_signed_cert_is_renewed(self, keyfile, crtfile): + """ + Self-signed certificate, no CA. + First create a cert that expires in 30 days, then recreate + the cert because the second state run requires days_remaining + to be at least 90. + """ + first_run = self.run_function( + "state.apply", + ["x509.self_signed_expiry"], + pillar={ + "keyfile": keyfile, + "crtfile": crtfile, + "days_valid": 30, + "days_remaining": 10, + }, + ) + key = "x509_|-self_signed_cert_|-{0}_|-certificate_managed".format(crtfile) + self.assertEqual( + "Certificate is valid and up to date", + first_run[key]["changes"]["Status"]["New"], + ) + expiry = datetime.datetime.strptime( + first_run[key]["changes"]["Certificate"]["New"]["Not After"], + "%Y-%m-%d %H:%M:%S", + ) + self.assertEqual(29, (expiry - datetime.datetime.now()).days) + self.assertTrue(os.path.exists(crtfile), "Certificate was not created.") + + with salt.utils.files.fopen(crtfile, "r") as first_cert: + cert_contents = first_cert.read() + + second_run = self.run_function( + "state.apply", + ["x509.self_signed_expiry"], + pillar={ + "keyfile": keyfile, + "crtfile": crtfile, + "days_valid": 180, + "days_remaining": 90, + }, + ) + self.assertEqual( + "Certificate needs renewal: 29 days remaining but it needs to be at least 90", + second_run[key]["changes"]["Status"]["Old"], + ) + expiry = datetime.datetime.strptime( + second_run[key]["changes"]["Certificate"]["New"]["Not After"], + "%Y-%m-%d %H:%M:%S", + ) + self.assertEqual(179, (expiry - datetime.datetime.now()).days) + with salt.utils.files.fopen(crtfile, "r") as second_cert: + self.assertNotEqual( + cert_contents, + second_cert.read(), + "Certificate contents should have changed.", ) From 916a48699f7a432d43a9cd85f693a4347024b784 Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 8 Feb 2020 15:40:38 +0100 Subject: [PATCH 04/10] Clean up imports and debug logging --- tests/integration/states/test_x509.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index 1ceb90a638d3..d321124c58e7 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -1,12 +1,11 @@ # -*- coding: utf-8 -*- from __future__ import absolute_import, unicode_literals import os -import logging import datetime +import textwrap import salt.utils.files from salt.ext import six -import textwrap from tests.support.helpers import with_tempfile from tests.support.case import ModuleCase @@ -22,9 +21,6 @@ HAS_M2CRYPTO = False -log = logging.getLogger(__name__) - - @skipIf(not HAS_M2CRYPTO, "Skip when no M2Crypto found") class x509Test(ModuleCase, SaltReturnAssertsMixin): @classmethod @@ -80,11 +76,6 @@ def tearDown(self): salt.utils.files.rm_rf(certs_path) self.run_function("saltutil.refresh_pillar") - def run_function(self, *args, **kwargs): # pylint: disable=arguments-differ - ret = super(x509Test, self).run_function(*args, **kwargs) - log.debug("ret = %s", ret) - return ret - @with_tempfile(suffix=".pem", create=False) def test_issue_49027(self, pemfile): ret = self.run_state("x509.pem_managed", name=pemfile, text=self.x509_cert_text) From 35a772f46549e0768cdde6b93e68347d99d01017 Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 8 Feb 2020 15:46:51 +0100 Subject: [PATCH 05/10] Use with_tempfile to prevent conflicts with other tests --- .../files/file/base/x509/self_signed.sls | 9 +++--- tests/integration/states/test_x509.py | 29 ++++++++++--------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/tests/integration/files/file/base/x509/self_signed.sls b/tests/integration/files/file/base/x509/self_signed.sls index 90fb5282dabe..11ca09877071 100644 --- a/tests/integration/files/file/base/x509/self_signed.sls +++ b/tests/integration/files/file/base/x509/self_signed.sls @@ -1,13 +1,14 @@ -{% set tmp_dir = pillar['tmp_dir'] %} +{% set keyfile = pillar['keyfile'] %} +{% set crtfile = pillar['crtfile'] %} private_key: x509.private_key_managed: - - name: {{ tmp_dir }}/self.key + - name: {{ keyfile }} self_signed_cert: x509.certificate_managed: - - name: {{ tmp_dir }}/self.crt - - signing_private_key: {{ tmp_dir }}/self.key + - name: {{ crtfile }} + - signing_private_key: {{ keyfile }} - CN: localhost - days_valid: 30 - days_remaining: 0 diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index d321124c58e7..4853f0f96444 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -112,36 +112,37 @@ def test_cert_signing(self): assert "Certificate" in ret[key]["changes"] assert "New" in ret[key]["changes"]["Certificate"] - def test_self_signed_cert(self): + @with_tempfile(suffix=".crt", create=False) + @with_tempfile(suffix=".key", create=False) + def test_self_signed_cert(self, keyfile, crtfile): """ Self-signed certificate, no CA. - Run the state twice to confirm the cert is only created once and its contents don't change. + Run the state twice to confirm the cert is only created once + and its contents don't change. """ first_run = self.run_function( - "state.apply", ["x509.self_signed"], pillar={"tmp_dir": RUNTIME_VARS.TMP} - ) - key = "x509_|-self_signed_cert_|-{}/self.crt_|-certificate_managed".format( - RUNTIME_VARS.TMP + "state.apply", + ["x509.self_signed"], + pillar={"keyfile": keyfile, "crtfile": crtfile}, ) + key = "x509_|-self_signed_cert_|-{}_|-certificate_managed".format(crtfile) self.assertIn("New", first_run[key]["changes"]["Certificate"]) self.assertEqual( "Certificate is valid and up to date", first_run[key]["changes"]["Status"]["New"], ) - cert_path = first_run[key]["name"] - self.assertTrue(os.path.exists(cert_path), "Certificate was not created.") + self.assertTrue(os.path.exists(crtfile), "Certificate was not created.") - with salt.utils.files.fopen(cert_path, "r") as first_cert: + with salt.utils.files.fopen(crtfile, "r") as first_cert: cert_contents = first_cert.read() second_run = self.run_function( - "state.apply", ["x509.self_signed"], pillar={"tmp_dir": RUNTIME_VARS.TMP} - ) - key = "x509_|-self_signed_cert_|-{}/self.crt_|-certificate_managed".format( - RUNTIME_VARS.TMP + "state.apply", + ["x509.self_signed"], + pillar={"keyfile": keyfile, "crtfile": crtfile}, ) self.assertEqual({}, second_run[key]["changes"]) - with salt.utils.files.fopen(cert_path, "r") as second_cert: + with salt.utils.files.fopen(crtfile, "r") as second_cert: self.assertEqual( cert_contents, second_cert.read(), From b2a9cf92b87c5e9e147ac8ad733c479368d85baa Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 8 Feb 2020 15:49:01 +0100 Subject: [PATCH 06/10] Include more info about days_remaining and append_certs --- salt/states/x509.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/salt/states/x509.py b/salt/states/x509.py index d0b71d9b6009..5b1b74fc3134 100644 --- a/salt/states/x509.py +++ b/salt/states/x509.py @@ -512,11 +512,15 @@ def certificate_managed(name, days_remaining=90, append_certs=None, **kwargs): Path to the certificate days_remaining : 90 - The minimum number of days remaining when the certificate should be - recreated. A value of 0 disables automatic renewal. + Recreate the certificate if the number of days remaining on it + are less than this number. The value should be less than + ``days_valid``, otherwise the certificate will be recreated + every time the state is run. A value of 0 disables automatic + renewal. append_certs: A list of certificates to be appended to the managed file. + They must be valid PEM files, otherwise an error will be thrown. kwargs: Any arguments supported by :py:func:`x509.create_certificate From 78c6f9ae5c1c17324511b353e5bd65cd19eef097 Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 28 Mar 2020 18:43:37 +0000 Subject: [PATCH 07/10] Test self signed certificates properties are handled correctly --- .../x509/self_signed_different_properties.sls | 18 +++++ tests/integration/states/test_x509.py | 74 ++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 tests/integration/files/file/base/x509/self_signed_different_properties.sls diff --git a/tests/integration/files/file/base/x509/self_signed_different_properties.sls b/tests/integration/files/file/base/x509/self_signed_different_properties.sls new file mode 100644 index 000000000000..919e8fd22ecd --- /dev/null +++ b/tests/integration/files/file/base/x509/self_signed_different_properties.sls @@ -0,0 +1,18 @@ +{% set keyfile = pillar['keyfile'] %} +{% set crtfile = pillar['crtfile'] %} +{% set subjectAltName = pillar['subjectAltName'] %} + +private_key: + x509.private_key_managed: + - name: {{ keyfile }} + +self_signed_cert: + x509.certificate_managed: + - name: {{ crtfile }} + - signing_private_key: {{ keyfile }} + - CN: service.local + - subjectAltName: {{ subjectAltName }} + - days_valid: 90 + - days_remaining: 30 + - require: + - x509: private_key diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index 4853f0f96444..00a3bbfb87b9 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -151,7 +151,7 @@ def test_self_signed_cert(self, keyfile, crtfile): @with_tempfile(suffix=".crt", create=False) @with_tempfile(suffix=".key", create=False) - def test_old_self_signed_cert_is_renewed(self, keyfile, crtfile): + def test_old_self_signed_cert_is_recreated(self, keyfile, crtfile): """ Self-signed certificate, no CA. First create a cert that expires in 30 days, then recreate @@ -208,3 +208,75 @@ def test_old_self_signed_cert_is_renewed(self, keyfile, crtfile): second_cert.read(), "Certificate contents should have changed.", ) + + @with_tempfile(suffix=".crt", create=False) + @with_tempfile(suffix=".key", create=False) + def test_mismatched_self_signed_cert_is_recreated(self, keyfile, crtfile): + """ + Self-signed certificate, no CA. + First create a cert, then run the state again with a different + subjectAltName. The cert should be recreated. + Finally, run once more with the same subjectAltName as the + second run. Nothing should change. + """ + first_run = self.run_function( + "state.apply", + ["x509.self_signed_different_properties"], + pillar={ + "keyfile": keyfile, + "crtfile": crtfile, + "subjectAltName": "DNS:alt.service.local", + }, + ) + key = "x509_|-self_signed_cert_|-{0}_|-certificate_managed".format(crtfile) + self.assertEqual( + "Certificate is valid and up to date", + first_run[key]["changes"]["Status"]["New"], + ) + sans = first_run[key]["changes"]["Certificate"]["New"]["X509v3 Extensions"][ + "subjectAltName" + ] + self.assertEqual("DNS:alt.service.local", sans) + self.assertTrue(os.path.exists(crtfile), "Certificate was not created.") + + with salt.utils.files.fopen(crtfile, "r") as first_cert: + first_cert_contents = first_cert.read() + + second_run_pillar = { + "keyfile": keyfile, + "crtfile": crtfile, + "subjectAltName": "DNS:alt1.service.local, DNS:alt2.service.local", + } + second_run = self.run_function( + "state.apply", + ["x509.self_signed_different_properties"], + pillar=second_run_pillar, + ) + self.assertEqual( + "Certificate properties are different: X509v3 Extensions", + second_run[key]["changes"]["Status"]["Old"], + ) + sans = second_run[key]["changes"]["Certificate"]["New"]["X509v3 Extensions"][ + "subjectAltName" + ] + self.assertEqual("DNS:alt1.service.local, DNS:alt2.service.local", sans) + with salt.utils.files.fopen(crtfile, "r") as second_cert: + second_cert_contents = second_cert.read() + self.assertNotEqual( + first_cert_contents, + second_cert_contents, + "Certificate contents should have changed.", + ) + + third_run = self.run_function( + "state.apply", + ["x509.self_signed_different_properties"], + pillar=second_run_pillar, + ) + self.assertEqual({}, third_run[key]["changes"]) + with salt.utils.files.fopen(crtfile, "r") as third_cert: + self.assertEqual( + second_cert_contents, + third_cert.read(), + "Certificate contents should not have changed.", + ) From 0ae7842fc2bd335f26c09cc0943dbc89403c22a1 Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 28 Mar 2020 19:19:35 +0000 Subject: [PATCH 08/10] Explicitly remove support for managed_private_key It doesn't work and hasn't for a long time, see https://github.com/saltstack/salt/issues/39608#issuecomment-342346894. Throwing a SaltInvocationError with a clear path to resolve the issue is more useful. --- salt/states/x509.py | 5 +++++ .../integration/files/file/base/test_cert.sls | 8 -------- tests/integration/states/test_x509.py | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/salt/states/x509.py b/salt/states/x509.py index 5b1b74fc3134..efa761681622 100644 --- a/salt/states/x509.py +++ b/salt/states/x509.py @@ -573,6 +573,11 @@ def certificate_managed(name, days_remaining=90, append_certs=None, **kwargs): "public_key or signing_private_key must be specified." ) + if "managed_private_key" in kwargs: + raise salt.exceptions.SaltInvocationError( + "managed_private_key is no longer supported by x509.certificate_managed, use a separate x509.private_key_managed call instead." + ) + ret = {"name": name, "result": False, "changes": {}, "comment": ""} is_valid, invalid_reason, current_cert_info = _certificate_is_valid( diff --git a/tests/integration/files/file/base/test_cert.sls b/tests/integration/files/file/base/test_cert.sls index a04142a47aa9..324b00565648 100644 --- a/tests/integration/files/file/base/test_cert.sls +++ b/tests/integration/files/file/base/test_cert.sls @@ -26,10 +26,6 @@ - days_valid: 3650 - days_remaining: 0 - backup: True - - managed_private_key: - name: {{ tmp_dir }}/pki/ca.key - bits: 4096 - backup: True - require: - file: {{ tmp_dir }}/pki - {{ tmp_dir }}/pki/ca.key @@ -56,10 +52,6 @@ test_crt: - CN: minion - days_remaining: 30 - backup: True - - managed_private_key: - name: {{ tmp_dir }}/pki/test.key - bits: 4096 - backup: True - require: - {{ tmp_dir }}/pki/ca.crt - {{ tmp_dir }}/pki/test.key diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index 00a3bbfb87b9..2a3352aa1962 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -280,3 +280,22 @@ def test_mismatched_self_signed_cert_is_recreated(self, keyfile, crtfile): third_cert.read(), "Certificate contents should not have changed.", ) + + @with_tempfile(suffix=".crt", create=False) + @with_tempfile(suffix=".key", create=False) + def test_managed_private_key_not_supported_by_certificate_managed( + self, keyfile, crtfile + ): + ret = self.run_state( + "x509.certificate_managed", + name=crtfile, + ca_server="any-minion-not-important", + signing_policy="not-important", + public_key=keyfile, + managed_private_key={"name": keyfile}, + ) + key = "x509_|-{0}_|-{0}_|-certificate_managed".format(crtfile) + expected = "managed_private_key is no longer supported by x509.certificate_managed, use a separate x509.private_key_managed call instead." + self.assertIn(expected, ret[key]["comment"], ret) + self.assertEqual(False, ret[key]["result"]) + self.assertEqual({}, ret[key]["changes"]) From d42f403576f0ed648375d41417314732006bb9fd Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Sat, 28 Mar 2020 19:27:46 +0000 Subject: [PATCH 09/10] Put test x509 state in the same directory as the others --- .../files/file/base/{test_cert.sls => x509/cert_signing.sls} | 0 tests/integration/states/test_x509.py | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename tests/integration/files/file/base/{test_cert.sls => x509/cert_signing.sls} (100%) diff --git a/tests/integration/files/file/base/test_cert.sls b/tests/integration/files/file/base/x509/cert_signing.sls similarity index 100% rename from tests/integration/files/file/base/test_cert.sls rename to tests/integration/files/file/base/x509/cert_signing.sls diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index 2a3352aa1962..6f60de09279e 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -102,7 +102,7 @@ def test_issue_49008(self, keyfile, crtfile): def test_cert_signing(self): ret = self.run_function( - "state.apply", ["test_cert"], pillar={"tmp_dir": RUNTIME_VARS.TMP} + "state.apply", ["x509.cert_signing"], pillar={"tmp_dir": RUNTIME_VARS.TMP} ) key = "x509_|-test_crt_|-{}/pki/test.crt_|-certificate_managed".format( RUNTIME_VARS.TMP From 76970539e732041fc5f8e9aba9a422288e1d0b70 Mon Sep 17 00:00:00 2001 From: Glynn Forrest Date: Thu, 9 Apr 2020 10:51:08 +0100 Subject: [PATCH 10/10] Log a warning instead of throwing an error for managed_private_key See https://github.com/saltstack/salt/pull/52935#discussion_r405839407 --- salt/states/x509.py | 21 ++++++++++++++------ tests/integration/states/test_x509.py | 28 ++++++++++++++------------- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/salt/states/x509.py b/salt/states/x509.py index efa761681622..ba8522607485 100644 --- a/salt/states/x509.py +++ b/salt/states/x509.py @@ -158,14 +158,16 @@ """ # Import Python Libs -from __future__ import absolute_import, unicode_literals, print_function +from __future__ import absolute_import, print_function, unicode_literals + +import copy import datetime import os import re -import copy # Import Salt Libs import salt.exceptions +import salt.utils.versions # Import 3rd-party libs from salt.ext import six @@ -504,7 +506,9 @@ def _certificate_is_valid(name, days_remaining, append_certs, **cert_spec): return False, "{0} is not a valid certificate: {1}".format(name, str(e)), {} -def certificate_managed(name, days_remaining=90, append_certs=None, **kwargs): +def certificate_managed( + name, days_remaining=90, append_certs=None, managed_private_key=None, **kwargs +): """ Manage a Certificate @@ -522,6 +526,10 @@ def certificate_managed(name, days_remaining=90, append_certs=None, **kwargs): A list of certificates to be appended to the managed file. They must be valid PEM files, otherwise an error will be thrown. + managed_private_key: + Has no effect since v2016.11 and will be removed in Salt Aluminium. + Use a separate x509.private_key_managed call instead. + kwargs: Any arguments supported by :py:func:`x509.create_certificate ` or :py:func:`file.managed @@ -573,9 +581,10 @@ def certificate_managed(name, days_remaining=90, append_certs=None, **kwargs): "public_key or signing_private_key must be specified." ) - if "managed_private_key" in kwargs: - raise salt.exceptions.SaltInvocationError( - "managed_private_key is no longer supported by x509.certificate_managed, use a separate x509.private_key_managed call instead." + if managed_private_key: + salt.utils.versions.warn_until( + "Aluminium", + "Passing 'managed_private_key' to x509.certificate_managed has no effect and will be removed Salt Aluminium. Use a separate x509.private_key_managed call instead.", ) ret = {"name": name, "result": False, "changes": {}, "comment": ""} diff --git a/tests/integration/states/test_x509.py b/tests/integration/states/test_x509.py index 6f60de09279e..ade554fbcc7d 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -1,17 +1,17 @@ # -*- coding: utf-8 -*- from __future__ import absolute_import, unicode_literals -import os + import datetime +import os import textwrap import salt.utils.files from salt.ext import six - -from tests.support.helpers import with_tempfile from tests.support.case import ModuleCase -from tests.support.unit import skipIf +from tests.support.helpers import with_tempfile from tests.support.mixins import SaltReturnAssertsMixin from tests.support.runtests import RUNTIME_VARS +from tests.support.unit import skipIf try: import M2Crypto # pylint: disable=W0611 @@ -283,19 +283,21 @@ def test_mismatched_self_signed_cert_is_recreated(self, keyfile, crtfile): @with_tempfile(suffix=".crt", create=False) @with_tempfile(suffix=".key", create=False) - def test_managed_private_key_not_supported_by_certificate_managed( + def test_certificate_managed_with_managed_private_key_does_not_error( self, keyfile, crtfile ): + """ + Test using the deprecated managed_private_key arg in certificate_managed does not throw an error. + + TODO: Remove this test in Aluminium when the arg is removed. + """ + self.run_state("x509.private_key_managed", name=keyfile, bits=4096) ret = self.run_state( "x509.certificate_managed", name=crtfile, - ca_server="any-minion-not-important", - signing_policy="not-important", - public_key=keyfile, - managed_private_key={"name": keyfile}, + CN="localhost", + signing_private_key=keyfile, + managed_private_key={"name": keyfile, "bits": 4096}, ) key = "x509_|-{0}_|-{0}_|-certificate_managed".format(crtfile) - expected = "managed_private_key is no longer supported by x509.certificate_managed, use a separate x509.private_key_managed call instead." - self.assertIn(expected, ret[key]["comment"], ret) - self.assertEqual(False, ret[key]["result"]) - self.assertEqual({}, ret[key]["changes"]) + self.assertEqual(True, ret[key]["result"])