From b4bb0a9574c3db8e9fc7b9a4c6f996c3cda9f20d Mon Sep 17 00:00:00 2001 From: Adriano Hernandez Date: Wed, 14 Aug 2019 18:33:37 -0700 Subject: [PATCH 1/5] Starting to make a test for key or value of encryption context too long throwing right type of serializationerror --- .../internal/formatting/encryption_context.py | 26 +++++++++++++------ test/unit/test_encryption_context.py | 4 +++ test/unit/test_serialize.py | 1 - 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/aws_encryption_sdk/internal/formatting/encryption_context.py b/src/aws_encryption_sdk/internal/formatting/encryption_context.py index 4d3a5e773..b30c794f7 100644 --- a/src/aws_encryption_sdk/internal/formatting/encryption_context.py +++ b/src/aws_encryption_sdk/internal/formatting/encryption_context.py @@ -82,15 +82,25 @@ def serialize_encryption_context(encryption_context): ) for key, value in sorted(encryption_context_list, key=lambda x: x[0]): - serialized_context.extend( - struct.pack( - ">H{key_size}sH{value_size}s".format(key_size=len(key), value_size=len(value)), - len(key), - key, - len(value), - value, + try: + serialized_context.extend( + struct.pack( + ">H{key_size}sH{value_size}s".format(key_size=len(key), value_size=len(value)), + len(key), + key, + len(value), + value, + ) ) - ) + # we check to make sure that we return the right type of error message for an overly long key or value + except struct.error as struct_error: + message = str(struct_error) + if message == "'H' format requires 0 <= number <= 65535": + # the key or value were too large + raise SerializationError("Key or Value are too large. Maximum is 2^16-1 (equivalent to \xff\xff).") + else: + # unknown struct error + raise struct_error if len(serialized_context) > aws_encryption_sdk.internal.defaults.MAX_BYTE_ARRAY_SIZE: raise SerializationError("The serialized context is too large.") return bytes(serialized_context) diff --git a/test/unit/test_encryption_context.py b/test/unit/test_encryption_context.py index 187365783..c35dd6fd6 100644 --- a/test/unit/test_encryption_context.py +++ b/test/unit/test_encryption_context.py @@ -190,3 +190,7 @@ def test_deserialize_encryption_context_empty(self): serialized_encryption_context=b"" ) assert test == {} + # important to not have autouse=true with a mock + def test_serialize_encryption_context_key_value_too_long(self): + dictionary = {} + serialize.serialize_encryption_context(dictionary) diff --git a/test/unit/test_serialize.py b/test/unit/test_serialize.py index 511048d80..539e9960e 100644 --- a/test/unit/test_serialize.py +++ b/test/unit/test_serialize.py @@ -48,7 +48,6 @@ def test_serialize_frame_invalid_sequence_number(sequence_number, error_message) excinfo.match(error_message) - class TestSerialize(object): @pytest.fixture(autouse=True) def apply_fixtures(self): From 1057912b9327a2b5adb4ce24327ca1b95ba1f73e Mon Sep 17 00:00:00 2001 From: Adriano Hernandez Date: Wed, 14 Aug 2019 18:59:38 -0700 Subject: [PATCH 2/5] Failing tests that will be succeeding once I change the logic --- test/unit/test_encryption_context.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test/unit/test_encryption_context.py b/test/unit/test_encryption_context.py index c35dd6fd6..460d16009 100644 --- a/test/unit/test_encryption_context.py +++ b/test/unit/test_encryption_context.py @@ -190,7 +190,19 @@ def test_deserialize_encryption_context_empty(self): serialized_encryption_context=b"" ) assert test == {} + # important to not have autouse=true with a mock + # these three tests make sure that whether a key or value or both are too long + # we throw the right type of serialization error and not the struct.error + # from struct.pack() + def test_serialize_encryption_context_key_too_long(self): + dictionary = {"0"*2**16:"short"} + aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) + + def test_serialize_encryption_context_value_too_long(self): + dictionary = {"short":"0"*2**16} + aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) + def test_serialize_encryption_context_key_value_too_long(self): - dictionary = {} - serialize.serialize_encryption_context(dictionary) + dictionary = {"0"*2**16:"0"*2**16} + aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) \ No newline at end of file From 99165795a5c9fc2e29cac5d369ec34128ce5351c Mon Sep 17 00:00:00 2001 From: Adriano Hernandez Date: Wed, 14 Aug 2019 19:46:54 -0700 Subject: [PATCH 3/5] Fixed issue 55 and now it throws the correct type of error. --- .../internal/formatting/encryption_context.py | 52 ++++++++++--------- test/unit/test_encryption_context.py | 17 ++---- test/unit/test_serialize.py | 1 + 3 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/aws_encryption_sdk/internal/formatting/encryption_context.py b/src/aws_encryption_sdk/internal/formatting/encryption_context.py index b30c794f7..4c3cfd08d 100644 --- a/src/aws_encryption_sdk/internal/formatting/encryption_context.py +++ b/src/aws_encryption_sdk/internal/formatting/encryption_context.py @@ -47,6 +47,33 @@ def assemble_content_aad(message_id, aad_content_string, seq_num, length): return struct.pack(fmt, message_id, aad_content_string.value, seq_num, length) +# help function to simplify code +# this is necessary to pass flake8 +def _serialize_encryption_context_list(encryption_context_list, serialized_context): + for key, value in sorted(encryption_context_list, key=lambda x: x[0]): + try: + serialized_context.extend( + struct.pack( + ">H{key_size}sH{value_size}s".format(key_size=len(key), value_size=len(value)), + len(key), + key, + len(value), + value, + ) + ) + # we check to make sure that we return the right type of error message for an overly long key or value + except struct.error as struct_error: + message = str(struct_error) + if message == "'H' format requires 0 <= number <= 65535": + # the key or value were too large + raise SerializationError("Key or Value are too large. Maximum length is 65535") + # else we can just raise the struct error as it was (unknown) + raise struct_error + if len(serialized_context) > aws_encryption_sdk.internal.defaults.MAX_BYTE_ARRAY_SIZE: + raise SerializationError("The serialized context is too large.") + return bytes(serialized_context) + + def serialize_encryption_context(encryption_context): """Serializes the contents of a dictionary into a byte string. @@ -80,30 +107,7 @@ def serialize_encryption_context(encryption_context): raise SerializationError( "Cannot encode dictionary key or value using {}.".format(aws_encryption_sdk.internal.defaults.ENCODING) ) - - for key, value in sorted(encryption_context_list, key=lambda x: x[0]): - try: - serialized_context.extend( - struct.pack( - ">H{key_size}sH{value_size}s".format(key_size=len(key), value_size=len(value)), - len(key), - key, - len(value), - value, - ) - ) - # we check to make sure that we return the right type of error message for an overly long key or value - except struct.error as struct_error: - message = str(struct_error) - if message == "'H' format requires 0 <= number <= 65535": - # the key or value were too large - raise SerializationError("Key or Value are too large. Maximum is 2^16-1 (equivalent to \xff\xff).") - else: - # unknown struct error - raise struct_error - if len(serialized_context) > aws_encryption_sdk.internal.defaults.MAX_BYTE_ARRAY_SIZE: - raise SerializationError("The serialized context is too large.") - return bytes(serialized_context) + return _serialize_encryption_context_list(encryption_context_list, serialized_context) def read_short(source, offset): diff --git a/test/unit/test_encryption_context.py b/test/unit/test_encryption_context.py index 460d16009..767b07882 100644 --- a/test/unit/test_encryption_context.py +++ b/test/unit/test_encryption_context.py @@ -191,18 +191,11 @@ def test_deserialize_encryption_context_empty(self): ) assert test == {} - # important to not have autouse=true with a mock - # these three tests make sure that whether a key or value or both are too long + # these three tests (in one function) make sure that whether a key or value or both are too long # we throw the right type of serialization error and not the struct.error # from struct.pack() - def test_serialize_encryption_context_key_too_long(self): - dictionary = {"0"*2**16:"short"} - aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) - - def test_serialize_encryption_context_value_too_long(self): - dictionary = {"short":"0"*2**16} - aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) - def test_serialize_encryption_context_key_value_too_long(self): - dictionary = {"0"*2**16:"0"*2**16} - aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) \ No newline at end of file + for dictionary in [{"0" * 2 ** 16: "short"}, {"short": "0" * 2 ** 16}, {"0" * 2 ** 16: "0" * 2 ** 16}]: + with pytest.raises(SerializationError) as excinfo: + aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) + excinfo.match(r"Key or Value are too large. Maximum length is 65535") diff --git a/test/unit/test_serialize.py b/test/unit/test_serialize.py index 539e9960e..511048d80 100644 --- a/test/unit/test_serialize.py +++ b/test/unit/test_serialize.py @@ -48,6 +48,7 @@ def test_serialize_frame_invalid_sequence_number(sequence_number, error_message) excinfo.match(error_message) + class TestSerialize(object): @pytest.fixture(autouse=True) def apply_fixtures(self): From c7743fd95cd52479c53cad97e2891a2d35cd1815 Mon Sep 17 00:00:00 2001 From: Adriano Hernandez Date: Thu, 15 Aug 2019 17:58:18 -0700 Subject: [PATCH 4/5] Did what Liz Roth mentioned in the comments to this PR. Formatted and improved readability and future compatibility. --- .../internal/formatting/encryption_context.py | 17 ++++++-- test/unit/test_encryption_context.py | 42 +++++++++++++++---- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/aws_encryption_sdk/internal/formatting/encryption_context.py b/src/aws_encryption_sdk/internal/formatting/encryption_context.py index 4c3cfd08d..a5e6f50e9 100644 --- a/src/aws_encryption_sdk/internal/formatting/encryption_context.py +++ b/src/aws_encryption_sdk/internal/formatting/encryption_context.py @@ -47,9 +47,15 @@ def assemble_content_aad(message_id, aad_content_string, seq_num, length): return struct.pack(fmt, message_id, aad_content_string.value, seq_num, length) -# help function to simplify code -# this is necessary to pass flake8 +def _key_or_value_too_large(encryption_context_list): + """Helper method to check whether a key or value are too long.""" + longest_key = max(encryption_context_list, key=lambda x: len(x[0]))[0] + longest_value = max(encryption_context_list, key=lambda x: len(x[1]))[1] + return len(longest_key) > 2 ** 16 - 1 or len(longest_value) > 2 ** 16 - 1 + + def _serialize_encryption_context_list(encryption_context_list, serialized_context): + """Helper function to serialize the list of encryption context key-value pairs.""" for key, value in sorted(encryption_context_list, key=lambda x: x[0]): try: serialized_context.extend( @@ -64,11 +70,14 @@ def _serialize_encryption_context_list(encryption_context_list, serialized_conte # we check to make sure that we return the right type of error message for an overly long key or value except struct.error as struct_error: message = str(struct_error) + # a key or value were too long and struct gave us the expected error if message == "'H' format requires 0 <= number <= 65535": - # the key or value were too large + raise SerializationError("Key or Value are too large. Maximum length is 65535") + # a key or value were too long and struct's error message is different (maybe an update) + if _key_or_value_too_large(encryption_context_list): raise SerializationError("Key or Value are too large. Maximum length is 65535") # else we can just raise the struct error as it was (unknown) - raise struct_error + raise SerializationError("Unknown Error with struct pack. Could not serialize key or value") if len(serialized_context) > aws_encryption_sdk.internal.defaults.MAX_BYTE_ARRAY_SIZE: raise SerializationError("The serialized context is too large.") return bytes(serialized_context) diff --git a/test/unit/test_encryption_context.py b/test/unit/test_encryption_context.py index 767b07882..f25ce62b8 100644 --- a/test/unit/test_encryption_context.py +++ b/test/unit/test_encryption_context.py @@ -11,6 +11,8 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. """Unit test suite for aws_encryption_sdk.internal.formatting.encryption_context""" +import struct + import pytest import aws_encryption_sdk.internal.defaults @@ -23,6 +25,23 @@ pytestmark = [pytest.mark.unit, pytest.mark.local] +def test_struct_behavior_pack_too_large(): + """ + If a key or value in the encryption context are too large we need to throw + a serialization error instead of the implicit struct.error caused by trying to + >H pack a key or value that is too long. The current implementation requires + the error to be thrown and checks for some conditions involving the size of a + key or value and the error message thrown. For it to work properly we need + struct to fail in a certain manner, thus this test. + """ + value = struct.pack(">H", 2 ** 16 - 1) + assert value + + with pytest.raises(struct.error) as excinfo: + struct.pack(">H", 2 ** 16) + excinfo.match(r"'H' format requires 0 <= number <= 65535") + + class TestEncryptionContext(object): def test_assemble_content_aad(self): """Validate that the assemble_content_aad function @@ -191,11 +210,20 @@ def test_deserialize_encryption_context_empty(self): ) assert test == {} - # these three tests (in one function) make sure that whether a key or value or both are too long - # we throw the right type of serialization error and not the struct.error - # from struct.pack() def test_serialize_encryption_context_key_value_too_long(self): - for dictionary in [{"0" * 2 ** 16: "short"}, {"short": "0" * 2 ** 16}, {"0" * 2 ** 16: "0" * 2 ** 16}]: - with pytest.raises(SerializationError) as excinfo: - aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) - excinfo.match(r"Key or Value are too large. Maximum length is 65535") + dictionary = {"0" * 2 ** 16: "0" * 2 ** 16} + with pytest.raises(SerializationError) as excinfo: + aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) + excinfo.match(r"Key or Value are too large. Maximum length is 65535") + + def test_serialize_encryption_context_value_too_long(self): + dictionary = {"short": "0" * 2 ** 16} + with pytest.raises(SerializationError) as excinfo: + aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) + excinfo.match(r"Key or Value are too large. Maximum length is 65535") + + def test_serialize_encryption_context_key_too_long(self): + dictionary = {"0" * 2 ** 16: "short"} + with pytest.raises(SerializationError) as excinfo: + aws_encryption_sdk.internal.formatting.encryption_context.serialize_encryption_context(dictionary) + excinfo.match(r"Key or Value are too large. Maximum length is 65535") From b693a53f3e59c402e1d77edc4d5fcc7c8bfb1c8a Mon Sep 17 00:00:00 2001 From: 18adrianoh Date: Sun, 18 Aug 2019 20:37:49 -0700 Subject: [PATCH 5/5] Second round of Liz's commends on issue #55. --- .../internal/formatting/encryption_context.py | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/aws_encryption_sdk/internal/formatting/encryption_context.py b/src/aws_encryption_sdk/internal/formatting/encryption_context.py index a5e6f50e9..61756f663 100644 --- a/src/aws_encryption_sdk/internal/formatting/encryption_context.py +++ b/src/aws_encryption_sdk/internal/formatting/encryption_context.py @@ -47,17 +47,13 @@ def assemble_content_aad(message_id, aad_content_string, seq_num, length): return struct.pack(fmt, message_id, aad_content_string.value, seq_num, length) -def _key_or_value_too_large(encryption_context_list): - """Helper method to check whether a key or value are too long.""" - longest_key = max(encryption_context_list, key=lambda x: len(x[0]))[0] - longest_value = max(encryption_context_list, key=lambda x: len(x[1]))[1] - return len(longest_key) > 2 ** 16 - 1 or len(longest_value) > 2 ** 16 - 1 - - def _serialize_encryption_context_list(encryption_context_list, serialized_context): """Helper function to serialize the list of encryption context key-value pairs.""" for key, value in sorted(encryption_context_list, key=lambda x: x[0]): try: + if len(key) > 2 ** 16 - 1 or len(value) > 2 ** 16 - 1: + raise SerializationError("Key or Value are too large. Maximum length is 65535") + serialized_context.extend( struct.pack( ">H{key_size}sH{value_size}s".format(key_size=len(key), value_size=len(value)), @@ -67,19 +63,19 @@ def _serialize_encryption_context_list(encryption_context_list, serialized_conte value, ) ) - # we check to make sure that we return the right type of error message for an overly long key or value except struct.error as struct_error: message = str(struct_error) - # a key or value were too long and struct gave us the expected error if message == "'H' format requires 0 <= number <= 65535": raise SerializationError("Key or Value are too large. Maximum length is 65535") - # a key or value were too long and struct's error message is different (maybe an update) - if _key_or_value_too_large(encryption_context_list): - raise SerializationError("Key or Value are too large. Maximum length is 65535") - # else we can just raise the struct error as it was (unknown) - raise SerializationError("Unknown Error with struct pack. Could not serialize key or value") + + # else unknown + raise SerializationError( + [SerializationError("Unknown Error with struct pack. Could not serialize key or value"), struct_error] + ) + if len(serialized_context) > aws_encryption_sdk.internal.defaults.MAX_BYTE_ARRAY_SIZE: raise SerializationError("The serialized context is too large.") + return bytes(serialized_context)