feat: Improvements to the message decryption process (#211)

See GHSA-89v2-g37m-g3ff
aws · May 27, 2021 · be80aa5 · be80aa5
1 parent ce1ecb0
commit be80aa5
Show file tree

Hide file tree

Showing 15 changed files with 754 additions and 58 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -2,6 +2,15 @@
 Changelog
 *********
 
+1.9.0 -- 2021-05-27
+===================
+
+Features
+--------
+* Improvements to the message decryption process
+
+  See https://github.com/aws/aws-encryption-sdk-cli/security/advisories/GHSA-89v2-g37m-g3ff.
+
 1.8.0 -- 2020-10-27
 ===================
 

diff --git a/README.rst b/README.rst
@@ -43,7 +43,7 @@ Required Prerequisites
 ======================
 
 * Python 2.7+ or 3.4+
-* aws-encryption-sdk >= 1.7.0
+* aws-encryption-sdk >= 1.9.0
 
 Installation
 ============
@@ -168,7 +168,7 @@ Metadata Contents
 `````````````````
 The metadata JSON contains the following fields:
 
-* ``"mode"`` : ``"encrypt"``/``"decrypt"``
+* ``"mode"`` : ``"encrypt"``/``"decrypt"``/``"decrypt-unsigned"``
 * ``"input"`` : Full path to input file (or ``"<stdin>"`` if stdin)
 * ``"output"`` : Full path to output file (or ``"<stdout>"`` if stdout)
 * ``"header"`` : JSON representation of `message header data`_
@@ -342,7 +342,6 @@ Allowed parameters:
 * **max_messages_encrypted** :  Determines how long each entry can remain in the cache, beginning when it was added.
 * **max_bytes_encrypted** : Specifies the maximum number of bytes that a cached data key can encrypt.
 
-
 Logging and Verbosity
 ---------------------
 The ``-v`` argument allows you to tune the verbosity of the built-in logging to your desired level.

diff --git a/requirements.txt b/requirements.txt
@@ -1,4 +1,4 @@
 base64io>=1.0.1
-aws-encryption-sdk~=1.7
+aws-encryption-sdk~=1.9
 setuptools
 attrs>=17.1.0
diff --git a/src/aws_encryption_sdk_cli/__init__.py b/src/aws_encryption_sdk_cli/__init__.py
@@ -173,6 +173,10 @@ def process_cli_request(stream_args, parsed_args):
         _LOGGER.warning("Invalid commitment policy: %s", parsed_args.commitment_policy)
         commitment_policy = CommitmentPolicy.FORBID_ENCRYPT_ALLOW_DECRYPT
 
+    # We don't want commitment policy to percolate through to be passed directly to the ESDK, since it is set
+    # via the ESDK Client object instead
+    stream_args.pop("commitment_policy", None)
+
     handler = IOHandler(
         metadata_writer=parsed_args.metadata_output,
         interactive=parsed_args.interactive,
@@ -182,6 +186,8 @@ def process_cli_request(stream_args, parsed_args):
         required_encryption_context=parsed_args.encryption_context,
         required_encryption_context_keys=parsed_args.required_encryption_context_keys,
         commitment_policy=commitment_policy,
+        buffer_output=parsed_args.buffer,
+        max_encrypted_data_keys=parsed_args.max_encrypted_data_keys,
     )
 
     if parsed_args.input == "-":
@@ -277,7 +283,6 @@ def cli(raw_args=None):
             )
 
         stream_args = stream_kwargs_from_args(args, crypto_materials_manager)
-
         process_cli_request(stream_args, args)
 
         return None

diff --git a/src/aws_encryption_sdk_cli/internal/arg_parsing.py b/src/aws_encryption_sdk_cli/internal/arg_parsing.py
@@ -23,7 +23,7 @@
 import aws_encryption_sdk
 import six
 
-from aws_encryption_sdk_cli.exceptions import ParameterParseError
+from aws_encryption_sdk_cli.exceptions import BadUserArgumentError, ParameterParseError
 from aws_encryption_sdk_cli.internal.identifiers import (
     ALGORITHM_NAMES,
     DEFAULT_MASTER_KEY_PROVIDER,
@@ -57,6 +57,21 @@
 _LOGGER = logging.getLogger(LOGGER_NAME)
 
 
+def _is_decrypt_mode(mode):
+    # type: (str) -> bool
+    """
+    Determines whether the provided mode does decryption
+
+    :param str filepath: Full file path to file in question
+    :rtype: bool
+    """
+    if mode in ("decrypt", "decrypt-unsigned"):
+        return True
+    if mode == "encrypt":
+        return False
+    raise BadUserArgumentError("Mode {mode} has not been implemented".format(mode=mode))
+
+
 class CommentIgnoringArgumentParser(argparse.ArgumentParser):
     """``ArgumentParser`` that ignores lines in ``fromfile_prefix_chars`` files which start with ``#``."""
 
@@ -202,6 +217,14 @@ def _build_parser():
         "-d", "--decrypt", dest="action", action="store_const", const="decrypt", help="Decrypt data"
     )
     parser.add_dummy_redirect_argument("--decrypt")
+    operating_action.add_argument(
+        "--decrypt-unsigned",
+        dest="action",
+        action="store_const",
+        const="decrypt-unsigned",
+        help="Decrypt data and enforce messages are unsigned during decryption.",
+    )
+    parser.add_dummy_redirect_argument("--decrypt-unsigned")
 
     # For each argument added to this group, a dummy redirect argument must
     # be added to the parent parser for each long form option string.
@@ -284,6 +307,10 @@ def _build_parser():
         ),
     )
 
+    parser.add_argument(
+        "-b", "--buffer", action="store_true", help="Buffer result in memory before releasing to output"
+    )
+
     parser.add_argument(
         "-i",
         "--input",
@@ -341,6 +368,13 @@ def _build_parser():
         ),
     )
 
+    parser.add_argument(
+        "--max-encrypted-data-keys",
+        type=int,
+        action=UniqueStoreAction,
+        help="Maximum number of encrypted data keys to wrap (during encryption) or to unwrap (during decryption)",
+    )
+
     parser.add_argument(
         "--suffix",
         nargs="?",
@@ -496,7 +530,7 @@ def _process_master_key_provider_configs(
     :raises ParameterParseError: if no key values are provided
     """
     if raw_keys is None:
-        if action == "decrypt":
+        if _is_decrypt_mode(action):
             # We allow not defining any master key provider configuration if decrypting with aws-kms.
             _LOGGER.debug(
                 "No master key provider config provided on decrypt request. Using aws-kms with no master keys."
@@ -529,7 +563,9 @@ def _process_master_key_provider_configs(
             )
         parsed_args["provider"] = provider[0]  # type: ignore
 
-        aws_kms_on_decrypt = parsed_args["provider"] in ("aws-kms", DEFAULT_MASTER_KEY_PROVIDER) and action == "decrypt"
+        aws_kms_on_decrypt = parsed_args["provider"] in ("aws-kms", DEFAULT_MASTER_KEY_PROVIDER) and _is_decrypt_mode(
+            action
+        )
 
         if aws_kms_on_decrypt:
             if "key" in parsed_args:
@@ -559,7 +595,7 @@ def _process_wrapping_key_provider_configs(  # noqa: C901
     :raises ParameterParseError: if no key values are provided
     """
     if raw_keys is None:
-        if action == "decrypt":
+        if _is_decrypt_mode(action):
             # We allow not defining any wrapping key provider configuration if decrypting with aws-kms.
             _LOGGER.debug(
                 "No wrapping key provider config provided on decrypt request. Using aws-kms with no wrapping keys."
@@ -592,7 +628,7 @@ def _process_wrapping_key_provider_configs(  # noqa: C901
         _process_discovery_args(parsed_args)
 
         discovery = parsed_args["discovery"]
-        if provider_is_kms and action == "decrypt":
+        if provider_is_kms and _is_decrypt_mode(action):
             if "key" in parsed_args and discovery:
                 # Decrypt MUST fail without attempting any decryption if discovery mode is enabled
                 # and at least one key=<Key ARN> parameter value is provided
@@ -713,15 +749,15 @@ def parse_args(raw_args=None):  # noqa
             raise ParameterParseError("You cannot specify both the --master-keys and --wrapping-keys parameters")
         if parsed_args.wrapping_keys:
             if not parsed_args.commitment_policy:
-                raise ParameterParseError('Commitment policy is required when specifying the --wrapping-keys parameter')
+                raise ParameterParseError("Commitment policy is required when specifying the --wrapping-keys parameter")
 
             parsed_args.wrapping_keys = _process_wrapping_key_provider_configs(
                 parsed_args.wrapping_keys, parsed_args.action
             )
         else:
             if parsed_args.commitment_policy:
                 raise ParameterParseError(
-                    'Commitment policy is only supported when using the --wrapping-keys parameter'
+                    "Commitment policy is only supported when using the --wrapping-keys parameter"
                 )
 
             _LOGGER.warning(

diff --git a/src/aws_encryption_sdk_cli/internal/identifiers.py b/src/aws_encryption_sdk_cli/internal/identifiers.py
@@ -31,10 +31,14 @@
     "DEFAULT_MASTER_KEY_PROVIDER",
     "OperationResult",
 )
-__version__ = "1.8.0"  # type: str
+__version__ = "1.9.0"  # type: str
 
 #: Suffix added to output files if specific output filename is not specified.
-OUTPUT_SUFFIX = {"encrypt": ".encrypted", "decrypt": ".decrypted"}  # type: Dict[str, str]
+OUTPUT_SUFFIX = {
+    "encrypt": ".encrypted",
+    "decrypt": ".decrypted",
+    "decrypt-unsigned": ".decrypted",
+}  # type: Dict[str, str]
 
 ALGORITHM_NAMES = {
     alg for alg in dir(aws_encryption_sdk.Algorithm) if not alg.startswith("_")

diff --git a/src/aws_encryption_sdk_cli/internal/io_handling.py b/src/aws_encryption_sdk_cli/internal/io_handling.py
@@ -24,6 +24,7 @@
 from aws_encryption_sdk.materials_managers import CommitmentPolicy
 from base64io import Base64IO
 
+from aws_encryption_sdk_cli.internal.arg_parsing import _is_decrypt_mode
 from aws_encryption_sdk_cli.internal.identifiers import OUTPUT_SUFFIX, OperationResult
 from aws_encryption_sdk_cli.internal.logging_utils import LOGGER_NAME
 from aws_encryption_sdk_cli.internal.metadata import MetadataWriter, json_ready_header, json_ready_header_auth
@@ -153,6 +154,7 @@ class IOHandler(object):
     :param bool no_overwrite: Should never overwrite existing files
     :param bool decode_input: Should input be base64 decoded before operation
     :param bool encode_output: Should output be base64 encoded after operation
+    :param bool buffer_output: Should buffer entire output before releasing to destination
     :param dict required_encryption_context: Encryption context key-value pairs to require
     :param list required_encryption_context_keys: Encryption context keys to require
     """
@@ -162,13 +164,14 @@ class IOHandler(object):
     no_overwrite = attr.ib(validator=attr.validators.instance_of(bool))
     decode_input = attr.ib(validator=attr.validators.instance_of(bool))
     encode_output = attr.ib(validator=attr.validators.instance_of(bool))
+    buffer_output = attr.ib(validator=attr.validators.instance_of(bool))
     required_encryption_context = attr.ib(validator=attr.validators.instance_of(dict))
     required_encryption_context_keys = attr.ib(
         validator=attr.validators.instance_of(list)
     )  # noqa pylint: disable=invalid-name
     client = attr.ib(validator=attr.validators.instance_of(aws_encryption_sdk.EncryptionSDKClient))
 
-    def __init__(
+    def __init__(  # noqa pylint: disable=too-many-arguments
         self,
         metadata_writer,  # type: MetadataWriter
         interactive,  # type: bool
@@ -178,6 +181,8 @@ def __init__(
         required_encryption_context,  # type: Dict[str, str]
         required_encryption_context_keys,  # type: List[str]
         commitment_policy,  # type: Union[CommitmentPolicy, None]
+        buffer_output,
+        max_encrypted_data_keys,  # type: Union[None, int]
     ):
         # type: (...) -> None
         """Workaround pending resolution of attrs/mypy interaction.
@@ -193,7 +198,11 @@ def __init__(
         self.required_encryption_context_keys = required_encryption_context_keys  # pylint: disable=invalid-name
         if not commitment_policy:
             commitment_policy = CommitmentPolicy.FORBID_ENCRYPT_ALLOW_DECRYPT
-        self.client = aws_encryption_sdk.EncryptionSDKClient(commitment_policy=commitment_policy)
+        self.buffer_output = buffer_output
+        self.client = aws_encryption_sdk.EncryptionSDKClient(
+            commitment_policy=commitment_policy,
+            max_encrypted_data_keys=max_encrypted_data_keys,
+        )
         attr.validate(self)
 
     def _single_io_write(self, stream_args, source, destination_writer):
@@ -226,7 +235,7 @@ def _single_io_write(self, stream_args, source, destination_writer):
                 else:
                     metadata_kwargs["header_auth"] = json_ready_header_auth(header_auth)
 
-                if stream_args["mode"] == "decrypt":
+                if _is_decrypt_mode(str(stream_args["mode"])):
                     discovered_ec = handler.header.encryption_context
                     missing_keys = set(self.required_encryption_context_keys).difference(set(discovered_ec.keys()))
                     missing_pairs = set(self.required_encryption_context.items()).difference(set(discovered_ec.items()))
@@ -246,9 +255,12 @@ def _single_io_write(self, stream_args, source, destination_writer):
                         return OperationResult.FAILED_VALIDATION
 
                 metadata.write_metadata(**metadata_kwargs)
-                for chunk in handler:
-                    _destination.write(chunk)
-                    _destination.flush()
+                if self.buffer_output:
+                    _destination.write(handler.read())
+                else:
+                    for chunk in handler:
+                        _destination.write(chunk)
+                        _destination.flush()
         return OperationResult.SUCCESS
 
     def process_single_operation(self, stream_args, source, destination):

diff --git a/test/integration/integration_test_utils.py b/test/integration/integration_test_utils.py
@@ -59,6 +59,20 @@ def cmk_arn():
 
 def encrypt_args_template(metadata=False, caching=False, encode=False, decode=False):
     template = "-e -i {source} -o {target} --encryption-context a=b c=d -m key=" + cmk_arn_value()
+    return _encrypt_args_modify_template(template, metadata, caching, encode, decode)
+
+
+def encrypt_args_template_wrapping(metadata=False, caching=False, encode=False, decode=False, append_commitment=False):
+    template_wrapping = "-e -i {source} -o {target} --encryption-context a=b c=d -w key=" + cmk_arn_value()
+    template_wrapping = _encrypt_args_modify_template(template_wrapping, metadata, caching, encode, decode)
+    if append_commitment:
+        template_wrapping += (
+            " --commitment-policy forbid-encrypt-allow-decrypt"  # required in 1.8 when using --wrapping-keys
+        )
+    return template_wrapping
+
+
+def _encrypt_args_modify_template(template, metadata=False, caching=False, encode=False, decode=False):
     if metadata:
         template += " {metadata}"
     else:
@@ -72,7 +86,7 @@ def encrypt_args_template(metadata=False, caching=False, encode=False, decode=Fa
     return template
 
 
-def decrypt_args_template(metadata=False, encode=False, decode=False):
+def decrypt_args_template(metadata=False, encode=False, decode=False, buffer=False):
     template = "-d -i {source} -o {target}"
     if metadata:
         template += " {metadata}"
@@ -82,6 +96,17 @@ def decrypt_args_template(metadata=False, encode=False, decode=False):
         template += " --encode"
     if decode:
         template += " --decode"
+    if buffer:
+        template += " --buffer"
+    return template
+
+
+def decrypt_unsigned_args_template(metadata=False):
+    template = "--decrypt-unsigned -i {source} -o {target}"
+    if metadata:
+        template += " {metadata}"
+    else:
+        template += " -S"
     return template