x509.certificate_managed: always call file.managed even when cert contents haven't changed

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
 

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 }}

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

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."
+        )