diff --git a/salt/states/x509.py b/salt/states/x509.py index 04b76f82c807..d5e1ba4b4c53 100644 --- a/salt/states/x509.py +++ b/salt/states/x509.py @@ -520,6 +520,25 @@ 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_file_managed(ret, file_args): + """ + Run file.managed and merge the result with an existing return dict. + The overall True/False result will be the result of the file.managed call. +""" + file_ret = __states__["file.managed"](**file_args) + + ret["result"] = file_ret["result"] + if ret["result"]: + ret["comment"] = "Certificate {0} is valid and up to date".format(ret["name"]) + else: + ret["comment"] = file_ret["comment"] + + if file_ret["changes"]: + ret["changes"] = {"File": file_ret["changes"]} + + return ret + + def certificate_managed( name, days_remaining=90, append_certs=None, managed_private_key=None, **kwargs ): @@ -619,11 +638,21 @@ def certificate_managed( ) if is_valid: - ret["result"] = True - ret["comment"] = "Certificate {0} is valid and up to date".format(name) - return ret + file_args, extra_args = _get_file_args(name, **kwargs) + + return _certificate_file_managed(ret, file_args) if __opts__["test"]: + file_args, extra_args = _get_file_args(name, **kwargs) + # Use empty contents for file.managed in test mode. + # We don't want generate a new certificate, even in memory, + # for security reasons. + # Using an empty string instead of omitting it will at least + # show the old certificate in the diff. + file_args["contents"] = "" + + ret = _certificate_file_managed(ret, file_args) + ret["result"] = None ret["comment"] = "Certificate {0} will be created".format(name) ret["changes"]["Status"] = { @@ -664,21 +693,18 @@ def certificate_managed( 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 = _certificate_file_managed(ret, file_args) - 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 + if ret["result"]: + 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", + } return ret 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 index 919e8fd22ecd..75867f768fed 100644 --- a/tests/integration/files/file/base/x509/self_signed_different_properties.sls +++ b/tests/integration/files/file/base/x509/self_signed_different_properties.sls @@ -1,6 +1,7 @@ {% set keyfile = pillar['keyfile'] %} {% set crtfile = pillar['crtfile'] %} -{% set subjectAltName = pillar['subjectAltName'] %} +{% set subjectAltName = pillar['subjectAltName']|default('DNS:alt.service.local') %} +{% set fileMode = pillar['fileMode']|default('0600') %} private_key: x509.private_key_managed: @@ -9,6 +10,7 @@ private_key: self_signed_cert: x509.certificate_managed: - name: {{ crtfile }} + - mode: {{ fileMode }} - signing_private_key: {{ keyfile }} - CN: service.local - subjectAltName: {{ subjectAltName }} diff --git a/tests/integration/files/file/base/x509/self_signed_file_error.sls b/tests/integration/files/file/base/x509/self_signed_file_error.sls new file mode 100644 index 000000000000..d412c1a9208c --- /dev/null +++ b/tests/integration/files/file/base/x509/self_signed_file_error.sls @@ -0,0 +1,20 @@ +{% set keyfile = pillar['keyfile'] %} +{% set crtfile = pillar['crtfile'] %} +{% set user = pillar['user'] %} + +private_key: + x509.private_key_managed: + - name: {{ keyfile }} + +self_signed_cert: + x509.certificate_managed: + - name: {{ crtfile }} + # crtfile is many folders deep, so this line will cause + # file.managed to fail + - makedirs: False + - signing_private_key: {{ keyfile }} + - CN: localhost + - 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 ff02550d0f9f..a9f75a3f65ea 100644 --- a/tests/integration/states/test_x509.py +++ b/tests/integration/states/test_x509.py @@ -478,3 +478,71 @@ def test_certificate_managed_with_managed_private_key_does_not_error( ) key = "x509_|-{0}_|-{0}_|-certificate_managed".format(crtfile) self.assertEqual(True, ret[key]["result"]) + + @with_tempfile(suffix=".crt", create=False) + @with_tempfile(suffix=".key", create=False) + def test_file_properties_are_updated(self, keyfile, crtfile): + """ + Self-signed certificate, no CA. + First create a cert, then run the state again with different + file mode. The cert should not be recreated, but the file + should be updated. + Finally, run once more with the same file mode as the second + run. Nothing should change. + """ + first_run = self.run_function( + "state.apply", + ["x509.self_signed_different_properties"], + pillar={"keyfile": keyfile, "crtfile": crtfile, "fileMode": "0755"}, + ) + 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"], + ) + self.assertTrue(os.path.exists(crtfile), "Certificate was not created.") + self.assertEqual("0755", oct(os.stat(crtfile).st_mode)[-4:]) + + second_run_pillar = { + "keyfile": keyfile, + "crtfile": crtfile, + "mode": "0600", + } + second_run = self.run_function( + "state.apply", + ["x509.self_signed_different_properties"], + pillar=second_run_pillar, + ) + self.assertEqual("0600", oct(os.stat(crtfile).st_mode)[-4:]) + + third_run = self.run_function( + "state.apply", + ["x509.self_signed_different_properties"], + pillar=second_run_pillar, + ) + self.assertEqual({}, third_run[key]["changes"]) + self.assertEqual("0600", oct(os.stat(crtfile).st_mode)[-4:]) + + @with_tempfile(suffix=".crt", create=False) + @with_tempfile(suffix=".key", create=False) + def test_file_managed_failure(self, keyfile, crtfile): + """ + Test that a failure in the file.managed call marks the state + call as failed. + """ + crtfile_pieces = os.path.split(crtfile) + bad_crtfile = os.path.join( + crtfile_pieces[0], "deeply/nested", crtfile_pieces[1] + ) + ret = self.run_function( + "state.apply", + ["x509.self_signed_file_error"], + pillar={"keyfile": keyfile, "crtfile": bad_crtfile}, + ) + + key = "x509_|-self_signed_cert_|-{0}_|-certificate_managed".format(bad_crtfile) + self.assertFalse(ret[key]["result"], "State should have failed.") + self.assertEqual({}, ret[key]["changes"]) + self.assertFalse( + os.path.exists(crtfile), "Certificate should not have been created." + )