Merge pull request #56372 from jdsieci/master-x509_fixes

Fix for #41858
2025-04-17 10:10:20 +00:00 · 2020-05-07 13:28:56 -07:00 · 2020-05-07 13:28:56 -07:00 · 42dccb8eed
commit 42dccb8eed
parent a6b63cb81d 20c7d3a9ea
8 changed files with 296 additions and 12 deletions
--- a/salt/modules/x509.py
+++ b/salt/modules/x509.py
@ -442,6 +442,23 @@ def _make_regex(pem_type):
    )


+def _valid_pem(pem, pem_type=None):
+    pem_type = "[0-9A-Z ]+" if pem_type is None else pem_type
+    _dregex = _make_regex(pem_type)
+    for _match in _dregex.finditer(pem):
+        if _match:
+            return _match
+    return None
+
+
+def _match_minions(test, minion):
+    if "@" in test:
+        match = __salt__["publish.publish"](tgt=minion, fun="match.compound", arg=test)
+        return match.get(minion, False)
+    else:
+        return __salt__["match.glob"](test, minion)
+
+
 def get_pem_entry(text, pem_type=None):
    """
    Returns a properly formatted PEM string from the input text fixing
@ -465,8 +482,6 @@ def get_pem_entry(text, pem_type=None):
    # Replace encoded newlines
    text = text.replace("\\n", "\n")

-    _match = None
-
    if (
        len(text.splitlines()) == 1
        and text.startswith("-----")
@ -490,19 +505,16 @@ def get_pem_entry(text, pem_type=None):
                    pem_temp = pem_temp[pem_temp.index("-") :]
        text = "\n".join(pem_fixed)

-    _dregex = _make_regex("[0-9A-Z ]+")
    errmsg = "PEM text not valid:\n{0}".format(text)
    if pem_type:
-        _dregex = _make_regex(pem_type)
        errmsg = "PEM does not contain a single entry of type {0}:\n" "{1}".format(
            pem_type, text
        )

-    for _match in _dregex.finditer(text):
-        if _match:
-            break
+    _match = _valid_pem(text, pem_type)
    if not _match:
        raise salt.exceptions.SaltInvocationError(errmsg)
+
    _match_dict = _match.groupdict()
    pem_header = _match_dict["pem_header"]
    proc_type = _match_dict["proc_type"]
@ -785,8 +797,8 @@ def write_pem(text, path, overwrite=True, pem_type=None):

        salt '*' x509.write_pem "-----BEGIN CERTIFICATE-----MIIGMzCCBBugA..." path=/etc/pki/mycert.crt
    """
+    text = get_pem_entry(text, pem_type=pem_type)
    with salt.utils.files.set_umask(0o077):
-        text = get_pem_entry(text, pem_type=pem_type)
        _dhparams = ""
        _private_key = ""
        if (
@ -1091,10 +1103,7 @@ def sign_remote_certificate(argdic, **kwargs):
    if "minions" in signing_policy:
        if "__pub_id" not in kwargs:
            return "minion sending this request could not be identified"
-        matcher = "match.glob"
-        if "@" in signing_policy["minions"]:
-            matcher = "match.compound"
-        if not __salt__[matcher](signing_policy["minions"], kwargs["__pub_id"]):
+        if not _match_minions(signing_policy["minions"], kwargs["__pub_id"]):
            return "{0} not permitted to use signing policy {1}".format(
                kwargs["__pub_id"], argdic["signing_policy"]
            )
@ -1356,6 +1365,19 @@ def create_certificate(path=None, text=False, overwrite=True, ca_server=None, **
        ``minions`` key is included in the signing policy, only minions
        matching that pattern (see match.glob and match.compound) will be
        permitted to remotely request certificates from that policy.
+        In order to ``match.compound`` to work salt master must peers permit
+        peers to call it.
+
+        Example:
+
+        /etc/salt/master.d/peer.conf
+
+        .. code-block:: yaml
+
+            peer:
+              .*:
+                - match.compound
+

        Example:

@ -1456,6 +1478,9 @@ def create_certificate(path=None, text=False, overwrite=True, ca_server=None, **
            )

        cert_txt = certs[ca_server]
+        if isinstance(cert_txt, str):
+            if not _valid_pem(cert_txt, "CERTIFICATE"):
+                raise salt.exceptions.SaltInvocationError(cert_txt)

        if path:
            return write_pem(
--- a/tests/integration/files/conf/master
+++ b/tests/integration/files/conf/master
@ -29,6 +29,7 @@ tcp_master_workers: 64515
 peer:
  '.*':
    - '(x509|test).*'
+    - 'match.*'

 ext_pillar:
  - ext_pillar_opts:
--- a/tests/integration/files/file/base/issue-41858/check.sls
+++ b/tests/integration/files/file/base/issue-41858/check.sls
@ -0,0 +1,16 @@
+test_priv_key:
+  x509.private_key_managed:
+    - name: {{ pillar['keyfile'] }}
+    - bits: 4096
+
+test_crt:
+  x509.certificate_managed:
+    - name: {{ pillar['crtfile'] }}
+    - public_key: {{ pillar['keyfile'] }}
+    - ca_server: minion
+    - signing_policy: {{ pillar['signing_policy'] }}
+    - CN: minion
+    - days_remaining: 30
+    - backup: True
+    - require:
+        - test_priv_key
--- a/tests/integration/files/file/base/issue-41858/gen_cert.sls
+++ b/tests/integration/files/file/base/issue-41858/gen_cert.sls
@ -0,0 +1,61 @@
+{% set tmp_dir = pillar['tmp_dir'] %}
+
+{{ tmp_dir }}/pki:
+  file.directory
+
+{{ tmp_dir  }}/pki/issued_certs:
+  file.directory
+
+{{ tmp_dir  }}/pki/ca.key:
+  x509.private_key_managed:
+    - bits: 4096
+    - require:
+      - file: {{ tmp_dir }}/pki
+
+{{ tmp_dir  }}/pki/ca.crt:
+  x509.certificate_managed:
+    - signing_private_key: {{ tmp_dir  }}/pki/ca.key
+    - CN: ca.example.com
+    - C: US
+    - ST: Utah
+    - L: Salt Lake City
+    - basicConstraints: "critical CA:true"
+    - keyUsage: "critical cRLSign, keyCertSign"
+    - subjectKeyIdentifier: hash
+    - authorityKeyIdentifier: keyid,issuer:always
+    - 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
+
+mine.send:
+  module.run:
+    - func: x509.get_pem_entries
+    - kwargs:
+        glob_path: {{ tmp_dir  }}/pki/ca.crt
+    - onchanges:
+      - x509: {{ tmp_dir  }}/pki/ca.crt
+
+test_priv_key:
+  x509.private_key_managed:
+    - name: {{ pillar['keyfile'] }}
+    - bits: 4096
+
+test_crt:
+  x509.certificate_managed:
+    - name: {{ pillar['crtfile'] }}
+    - public_key: {{ pillar['keyfile'] }}
+    - ca_server: minion
+    - signing_policy: ca_policy
+    - CN: minion
+    - days_remaining: 30
+    - backup: True
+    - require:
+        - {{ tmp_dir  }}/pki/ca.crt
+        - test_priv_key
--- a/tests/integration/files/file/base/x509_compound_match/check.sls
+++ b/tests/integration/files/file/base/x509_compound_match/check.sls
@ -0,0 +1,16 @@
+test_priv_key:
+  x509.private_key_managed:
+    - name: {{ pillar['keyfile'] }}
+    - bits: 4096
+
+test_crt:
+  x509.certificate_managed:
+    - name: {{ pillar['crtfile'] }}
+    - public_key: {{ pillar['keyfile'] }}
+    - ca_server: minion
+    - signing_policy: {{ pillar['signing_policy'] }}
+    - CN: {{ grains.get('id') }}
+    - days_remaining: 30
+    - backup: True
+    - require:
+        - test_priv_key
--- a/tests/integration/files/file/base/x509_compound_match/gen_ca.sls
+++ b/tests/integration/files/file/base/x509_compound_match/gen_ca.sls
@ -0,0 +1,43 @@
+{% set tmp_dir = pillar['tmp_dir'] %}
+
+{{ tmp_dir }}/pki:
+  file.directory
+
+{{ tmp_dir  }}/pki/issued_certs:
+  file.directory
+
+{{ tmp_dir  }}/pki/ca.key:
+  x509.private_key_managed:
+    - bits: 4096
+    - require:
+      - file: {{ tmp_dir }}/pki
+
+{{ tmp_dir  }}/pki/ca.crt:
+  x509.certificate_managed:
+    - signing_private_key: {{ tmp_dir  }}/pki/ca.key
+    - CN: ca.example.com
+    - C: US
+    - ST: Utah
+    - L: Salt Lake City
+    - basicConstraints: "critical CA:true"
+    - keyUsage: "critical cRLSign, keyCertSign"
+    - subjectKeyIdentifier: hash
+    - authorityKeyIdentifier: keyid,issuer:always
+    - 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
+
+mine.send:
+  module.run:
+    - func: x509.get_pem_entries
+    - kwargs:
+        glob_path: {{ tmp_dir  }}/pki/ca.crt
+    - onchanges:
+      - x509: {{ tmp_dir  }}/pki/ca.crt
--- 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 datetime
+import hashlib
 import os
 import textwrap

@ -49,6 +50,18 @@ class x509Test(ModuleCase, SaltReturnAssertsMixin):
                    - authorityKeyIdentifier: keyid
                    - days_valid: 730
                    - copypath: {0}/pki
+                  compound_match:
+                    - minions: 'G@x509_test_grain:correct_value'
+                    - signing_private_key: {0}/pki/ca.key
+                    - signing_cert: {0}/pki/ca.crt
+                    - O: Test Company
+                    - basicConstraints: "CA:false"
+                    - keyUsage: "critical digitalSignature, keyEncipherment"
+                    - extendedKeyUsage: "critical serverAuth, clientAuth"
+                    - subjectKeyIdentifier: hash
+                    - authorityKeyIdentifier: keyid
+                    - days_valid: 730
+                    - copypath: {0}/pki
                     """.format(
                        RUNTIME_VARS.TMP
                    )
@ -67,6 +80,12 @@ class x509Test(ModuleCase, SaltReturnAssertsMixin):
                )
            )
        self.run_function("saltutil.refresh_pillar")
+        self.run_function(
+            "grains.set", ["x509_test_grain", "correct_value"], minion_tgt="sub_minion"
+        )
+        self.run_function(
+            "grains.set", ["x509_test_grain", "not_correct_value"], minion_tgt="minion"
+        )

    def tearDown(self):
        os.remove(os.path.join(RUNTIME_VARS.TMP_PILLAR_TREE, "signing_policies.sls"))
@ -75,6 +94,20 @@ class x509Test(ModuleCase, SaltReturnAssertsMixin):
        if os.path.exists(certs_path):
            salt.utils.files.rm_rf(certs_path)
        self.run_function("saltutil.refresh_pillar")
+        self.run_function("grains.delkey", ["x509_test_grain"], minion_tgt="sub_minion")
+        self.run_function("grains.delkey", ["x509_test_grain"], minion_tgt="minion")
+
+    def run_function(self, *args, **kwargs):  # pylint: disable=arguments-differ
+        ret = super(x509Test, self).run_function(*args, **kwargs)
+        return ret
+
+    @staticmethod
+    def file_checksum(path):
+        hash = hashlib.sha1()
+        with salt.utils.files.fopen(path, "rb") as f:
+            for block in iter(lambda: f.read(4096), b""):
+                hash.update(block)
+        return hash.hexdigest()

    @with_tempfile(suffix=".pem", create=False)
    @slowTest
@ -169,6 +202,93 @@ class x509Test(ModuleCase, SaltReturnAssertsMixin):
        not_after = ret[key]["changes"]["Certificate"]["New"]["Not After"]
        assert not_after == "2020-05-05 14:30:00"

+    @with_tempfile(suffix=".crt", create=False)
+    @with_tempfile(suffix=".key", create=False)
+    def test_issue_41858(self, keyfile, crtfile):
+        ret_key = "x509_|-test_crt_|-{0}_|-certificate_managed".format(crtfile)
+        signing_policy = "no_such_policy"
+        ret = self.run_function(
+            "state.apply",
+            ["issue-41858.gen_cert"],
+            pillar={
+                "keyfile": keyfile,
+                "crtfile": crtfile,
+                "tmp_dir": RUNTIME_VARS.TMP,
+            },
+        )
+        self.assertTrue(ret[ret_key]["result"])
+        cert_sum = self.file_checksum(crtfile)
+
+        ret = self.run_function(
+            "state.apply",
+            ["issue-41858.check"],
+            pillar={
+                "keyfile": keyfile,
+                "crtfile": crtfile,
+                "signing_policy": signing_policy,
+            },
+        )
+        self.assertFalse(ret[ret_key]["result"])
+        # self.assertSaltCommentRegexpMatches(ret[ret_key], "Signing policy {0} does not exist".format(signing_policy))
+        self.assertEqual(self.file_checksum(crtfile), cert_sum)
+
+    @with_tempfile(suffix=".crt", create=False)
+    @with_tempfile(suffix=".key", create=False)
+    def test_compound_match_minion_have_correct_grain_value(self, keyfile, crtfile):
+        ret_key = "x509_|-test_crt_|-{0}_|-certificate_managed".format(crtfile)
+        signing_policy = "compound_match"
+        ret = self.run_function(
+            "state.apply",
+            ["x509_compound_match.gen_ca"],
+            pillar={"tmp_dir": RUNTIME_VARS.TMP},
+        )
+
+        # sub_minion have grain set and CA is on other minion
+        # CA minion have same grain with incorrect value
+        ret = self.run_function(
+            "state.apply",
+            ["x509_compound_match.check"],
+            minion_tgt="sub_minion",
+            pillar={
+                "keyfile": keyfile,
+                "crtfile": crtfile,
+                "signing_policy": signing_policy,
+            },
+        )
+        self.assertTrue(ret[ret_key]["result"])
+
+    @with_tempfile(suffix=".crt", create=False)
+    @with_tempfile(suffix=".key", create=False)
+    def test_compound_match_ca_have_correct_grain_value(self, keyfile, crtfile):
+        self.run_function(
+            "grains.set", ["x509_test_grain", "correct_value"], minion_tgt="minion"
+        )
+        self.run_function(
+            "grains.set",
+            ["x509_test_grain", "not_correct_value"],
+            minion_tgt="sub_minion",
+        )
+
+        ret_key = "x509_|-test_crt_|-{0}_|-certificate_managed".format(crtfile)
+        signing_policy = "compound_match"
+        self.run_function(
+            "state.apply",
+            ["x509_compound_match.gen_ca"],
+            pillar={"tmp_dir": RUNTIME_VARS.TMP},
+        )
+
+        ret = self.run_function(
+            "state.apply",
+            ["x509_compound_match.check"],
+            minion_tgt="sub_minion",
+            pillar={
+                "keyfile": keyfile,
+                "crtfile": crtfile,
+                "signing_policy": signing_policy,
+            },
+        )
+        self.assertFalse(ret[ret_key]["result"])
+
    @with_tempfile(suffix=".crt", create=False)
    @with_tempfile(suffix=".key", create=False)
    def test_self_signed_cert(self, keyfile, crtfile):
--- a/tests/support/case.py
+++ b/tests/support/case.py
@ -693,6 +693,8 @@ class ModuleCase(TestCase, SaltClientTestCaseMixin):
            "pkg.refresh_db",
            "ssh.recv_known_host_entries",
            "time.sleep",
+            "grains.delkey",
+            "grains.delval",
        )
        if "f_arg" in kwargs:
            kwargs["arg"] = kwargs.pop("f_arg")