Fix multiple I/O issues in cert generation

This patch handles the case where CA certs or authorities are loaded as
byte strings. It also disables parsing YAML documents with python/object
types directly into (non-dict) Python objects (which is PyYaml's
default behavior), as it creates issues with the PeglegManagedDocument
module.
The patch also fixes a bug where attempting to re-encrypt an already
encrypted file would result in a serialized python object being written
rather than the expected output YAML.

Change-Id: I4b84ee8f9922ae042411e70242ffda4622647e86

pegleg/engine/catalog/pki_utility.py

@@ -26,6 +26,7 @@ import pytz
 import yaml
 
 from pegleg.engine import exceptions
+from pegleg.engine.util.catalog import decode_bytes
 from pegleg.engine.util.pegleg_managed_document import \
     PeglegManagedSecretsDocument
 
@@ -227,15 +228,14 @@ class PKIUtility(object):
         with tempfile.TemporaryDirectory() as tmp:
             for filename, data in files.items():
                 with open(os.path.join(tmp, filename), 'w') as f:
-                    f.write(data)
+                    f.write(decode_bytes(data))
 
             # Ignore bandit false positive:
             #   B603:subprocess_without_shell_equals_true
             # This method wraps cfssl calls originating from this module.
             result = subprocess.check_output(  # nosec
                 ['cfssl'] + command, cwd=tmp, stderr=subprocess.PIPE)
-            if not isinstance(result, str):
+            result = decode_bytes(result)
-                result = result.decode('utf-8')
             return json.loads(result)
 
     def _openssl(self, command, *, files=None):
@@ -246,7 +246,7 @@ class PKIUtility(object):
         with tempfile.TemporaryDirectory() as tmp:
             for filename, data in files.items():
                 with open(os.path.join(tmp, filename), 'w') as f:
-                    f.write(data)
+                    f.write(decode_bytes(data))
 
             # Ignore bandit false positive:
             #   B603:subprocess_without_shell_equals_true

pegleg/engine/site.py

@@ -18,6 +18,7 @@ import os
 import click
 import git
 import yaml
+from yaml.constructor import SafeConstructor
 
 from prettytable import PrettyTable
 
@@ -99,6 +100,9 @@ def collect(site_name, save_location):
 
 def render(site_name, output_stream, validate):
     documents = []
+    # Ignore YAML tags, only construct dicts
+    SafeConstructor.add_multi_constructor(
+        '', lambda loader, suffix, node: None)
     for filename in util.definition.site_files(site_name):
         with open(filename) as f:
             documents.extend(list(yaml.safe_load_all(f)))

pegleg/engine/util/catalog.py

@@ -19,7 +19,23 @@ from pegleg.engine.util import definition
 
 LOG = logging.getLogger(__name__)
 
-__all__ = ('iterate', )
+__all__ = ('iterate', 'decode_bytes')
+
+
+def decode_bytes(obj):
+    """If the argument is bytes, decode it.
+
+    :param Object obj: A string or byte object
+    :return: A string representation of obj
+    :rtype: str
+
+    """
+    if isinstance(obj, bytes):
+        return obj.decode('utf-8')
+    elif isinstance(obj, str):
+        return obj
+    else:
+        raise ValueError("ERROR: {} is not bytes or a string.".format(obj))
 
 
 def iterate(kind, sitename=None, documents=None):

pegleg/engine/util/files.py

@@ -18,6 +18,7 @@ import os
 
 import click
 import yaml
+from yaml.constructor import SafeConstructor
 
 from pegleg import config
 from pegleg.engine import util
@@ -227,6 +228,9 @@ def slurp(path):
 
     with open(path) as f:
         try:
+            # Ignore YAML tags, only construct dicts
+            SafeConstructor.add_multi_constructor(
+                '', lambda loader, suffix, node: None)
             return yaml.safe_load(f)
         except Exception as e:
             raise click.ClickException('Failed to parse %s:\n%s' % (path, e))
@@ -277,10 +281,14 @@ def read(path):
             document)
 
     with open(path) as stream:
+        # Ignore YAML tags, only construct dicts
+        SafeConstructor.add_multi_constructor(
+            '', lambda loader, suffix, node: None)
         try:
             return [
                 d for d in yaml.safe_load_all(stream)
-                if is_deckhand_document(d) or is_pegleg_managed_document(d)
+                if d and (is_deckhand_document(d) or
+                          is_pegleg_managed_document(d))
             ]
         except yaml.YAMLError as e:
             raise click.ClickException('Failed to parse %s:\n%s' % (path, e))

pegleg/engine/util/pegleg_secret_management.py

@@ -154,7 +154,7 @@ class PeglegSecretManagement(object):
         for doc in self.documents:
             # do not re-encrypt already encrypted data
             if doc.is_encrypted():
-                doc_list.append(doc)
+                doc_list.append(doc.pegleg_document)
                 continue
 
             # only encrypt if storagePolicy is set to encrypted.

requirements.txt

@@ -1,7 +1,7 @@
 gitpython
 click==6.7
 jsonschema==2.6.0
-pyyaml==3.12
+pyyaml==5.1
 cryptography==2.3.1
 python-dateutil==2.7.3
 

tests/unit/engine/test_secrets.py

@@ -18,13 +18,13 @@ from os import listdir
 import click
 import mock
 import pytest
-import yaml
 import tempfile
+import yaml
 
 from pegleg import config
-from pegleg.engine import secrets
-from pegleg.engine.catalog import pki_utility
 from pegleg.engine.catalog.pki_generator import PKIGenerator
+from pegleg.engine.catalog import pki_utility
+from pegleg.engine import secrets
 from pegleg.engine.util import encryption as crypt, catalog, git
 from pegleg.engine.util import files
 from pegleg.engine.util.pegleg_managed_document import \
@@ -120,7 +120,7 @@ data: {0}-password
 
 
 def test_pegleg_secret_management_constructor():
-    test_data = yaml.load(TEST_DATA)
+    test_data = yaml.safe_load(TEST_DATA)
     doc = PeglegManagedSecretsDocument(test_data)
     assert doc.is_storage_policy_encrypted()
     assert not doc.is_encrypted()
@@ -157,6 +157,18 @@ def test_pegleg_secret_management_constructor_with_invalid_arguments():
            'specified.' in str(err_info.value)
 
 
+@mock.patch.dict(os.environ, {
+    ENV_PASSPHRASE: 'ytrr89erARAiPE34692iwUMvWqqBvC',
+    ENV_SALT: 'MySecretSalt1234567890]['
+})
+def test_pegleg_secret_management_double_encrypt():
+    encrypted_doc = PeglegSecretManagement(
+        docs=[yaml.safe_load(TEST_DATA)]).get_encrypted_secrets()[0][0]
+    encrypted_doc_2 = PeglegSecretManagement(
+        docs=[encrypted_doc]).get_encrypted_secrets()[0][0]
+    assert encrypted_doc == encrypted_doc_2
+
+
 @mock.patch.dict(os.environ, {
     ENV_PASSPHRASE: 'ytrr89erARAiPE34692iwUMvWqqBvC',
     ENV_SALT: 'MySecretSalt1234567890]['
@@ -236,7 +248,8 @@ def test_generate_pki_using_local_repo_path(create_tmp_deployment_files):
     repo_path = str(git.git_handler(TEST_PARAMS["repo_url"],
                                     ref=TEST_PARAMS["repo_rev"]))
     with mock.patch.dict(config.GLOBAL_CONTEXT, {"site_repo": repo_path}):
-        pki_generator = PKIGenerator(duration=365, sitename=TEST_PARAMS["site_name"])
+        pki_generator = PKIGenerator(duration=365,
+                                     sitename=TEST_PARAMS["site_name"])
         generated_files = pki_generator.generate()
 
         assert len(generated_files), 'No secrets were generated'
@@ -258,7 +271,8 @@ def test_check_expiry(create_tmp_deployment_files):
     repo_path = str(git.git_handler(TEST_PARAMS["repo_url"],
                                     ref=TEST_PARAMS["repo_rev"]))
     with mock.patch.dict(config.GLOBAL_CONTEXT, {"site_repo": repo_path}):
-        pki_generator = PKIGenerator(duration=365, sitename=TEST_PARAMS["site_name"])
+        pki_generator = PKIGenerator(duration=365,
+                                     sitename=TEST_PARAMS["site_name"])
         generated_files = pki_generator.generate()
 
         pki_util = pki_utility.PKIUtility(duration=0)

tests/unit/engine/util/test_files.py

@@ -20,6 +20,7 @@ import yaml
 from pegleg import config
 from pegleg.engine.util import files
 from tests.unit.fixtures import create_tmp_deployment_files
+from tests.unit.fixtures import temp_path
 
 
 class TestFileHelpers(object):
@@ -65,3 +66,18 @@ def test_file_in_subdir():
     assert not files.file_in_subdir("aaa/bbb/ccc.txt", "ccc")
     assert not files.file_in_subdir("aaa/bbb/ccc.txt", "bb")
     assert not files.file_in_subdir("aaa/bbb/../ccc.txt", "bbb")
+
+
+def test_read(temp_path):
+    # This will throw an error if yaml attempts to read the tag.
+    with open(os.path.join(temp_path, "invalid.yaml"), "w") as invalid_yaml:
+        invalid_yaml.write("!!python/name:fake_class''\n")
+        files.read(os.path.join(temp_path, "invalid.yaml"))
+
+    # Under PyYAML's default behavior, the tag !!python/name:builtins.int
+    # will be parsed into the method int. files.read should ignore this tag.
+    with open(os.path.join(temp_path, "valid.yaml"), "w") as valid_yaml:
+        valid_yaml.write("!!python/name:builtins.int ''\n")
+    read_files = files.read(os.path.join(temp_path, "valid.yaml"))
+    # Assert that the tag was not parsed into the method int
+    assert int not in read_files