docs: specify how caching CMM should handle max plaintext length in Get Encryption Materials #159

alex-chew · 2020-07-15T16:28:07Z

Issue #, if available: #80 and #81

Description of changes:

The Get Encryption Materials operation accepts an optional max plaintext length parameter, but the specification does not state how the caching CMM should behave when the caller does not provide the parameter value. This change specifies that the caching CMM should bypass the cache in this case.

Also, when the caching CMM is performing a Get Encryption Materials operation for which no materials are cached, it MUST call its underlying CMM's Get Encryption Materials operation. The specification does not state what value of max plaintext length (if any) the caching CMM should pass to its underlying CMM. This change specifies that the caching CMM should pass its byte limit value as the max plaintext length parameter of the call to the underlying CMM.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…t Encryption Materials

lavaleri

Mostly looks good! Some comments.

framework/caching-cmm.md

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

lavaleri · 2020-07-15T18:40:48Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+and caches the materials that it receives.
+If a later Get Encryption Materials operation fetches those materials from the cache,
+but has a `max plaintext length` value that exceeds `N`,
+then the caching CMM returns those materials unsafely.


What do we mean by "unsafely"? We should say exactly what we mean here. By passing N to the underlying CMM, this CMM is signaling to the underlying CMM that it intends to only use the returned materials to encrypt up to N bytes. By then reusing those materials such that when it reuses the materials it encrypts plaintext with length exceeding N, this CMM is violating it's previous intent. This violation is why this supposed approach is bad.

Is there an edge case here is 2 CMMs share a CMC (and entries via partitions) but have different max bytes?
If I'm right, is this something to worry about? I don't think that the byte limit passed to the CMM is include in the entry, perhaps it should?

Say:

CMM A max bytes = 100K
CMM B max bytes = 200K

CMM A puts in an entry but then will never touch it again.
CMM B will use the entry to exhaustion.

@lavaleri thanks, I've incorporated your wording to clarify the explanation.

After a discussion with @lavaleri, @seebees, and @mattsb42-aws offline, we think that the edge case that @seebees described requires more thought to resolve properly, and doing so may require changes to other parts of this PR. So we are moving the PR to a later milestone.

acioc

Looks good. +1 to @lavaleri 's comments.

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

acioc · 2020-07-15T20:35:14Z

framework/caching-cmm.md

+| Python     | 0.1.0-preview                          | 1.3.0                     | [materials_managers/caching.py](https://github.com/aws/aws-encryption-sdk-python/blob/master/src/aws_encryption_sdk/materials_managers/caching.py)                                              |
+| Java       | 0.1.0-preview                          | 1.3.0                     | [CachingCryptoMaterialsManager.java](https://github.com/aws/aws-encryption-sdk-java/blob/master/src/main/java/com/amazonaws/encryptionsdk/caching/CachingCryptoMaterialsManager.java)           |


To make sure I understand this, is 1.3.0 the version of the ESDK? So Python and Java are on 1.3.0 while C and JS are both on 0.1.0?

As I understand it, these lines state that the Python ESDK version 1.3.0 is the minimum version that is confirmed to be compatible with spec version 0.1.0-preview, and likewise for the Java ESDK. Similarly, the C ESDK version 0.1.0 is the minimum version that is confirmed to be compatible with spec 0.1.0-preview, and likewise for the JS ESDKs.

acioc · 2020-07-15T20:36:55Z

Quick follow up. Make sure not-grep and prettier both work. I suspect they're pending because of the force-push.

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

seebees · 2020-07-16T15:19:17Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+and caches the materials that it receives.
+If a later Get Encryption Materials operation fetches those materials from the cache,
+but has a `max plaintext length` value that exceeds `N`,
+then the caching CMM returns those materials unsafely.


Is there an edge case here is 2 CMMs share a CMC (and entries via partitions) but have different max bytes?
If I'm right, is this something to worry about? I don't think that the byte limit passed to the CMM is include in the entry, perhaps it should?

Say:

CMM A max bytes = 100K
CMM B max bytes = 200K

CMM A puts in an entry but then will never touch it again.
CMM B will use the entry to exhaustion.

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

mattsb42-aws · 2020-07-16T19:52:54Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+This change will break any customer use case
+that depends on the Caching CMM passing no `max plaintext length` parameter
+to the underlying CMM on Get Encryption Materials calls.


This is a good call-out, but you don't actually mention anywhere that this is what implementations currently do.

mattsb42-aws · 2020-07-16T19:59:20Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+
+## Security Implications
+
+This change SHOULD NOT have any security implications.


Because our current implementations do not pass a max plaintext length value to the underlying CMM, and this is changing to now pass the byte limit value instead, the security implications for the value received from the underlying CMM are changing. This is both a security and operational change, as the underlying CMM could be making security decisions based on the max plaintext length value (or absence); for example if it is another caching CMM.

mattsb42-aws · 2020-07-16T21:00:37Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+## Guide-level Explanation
+
+When making a Get Encryption Materials call to the caching CMM,
+the user may optionally specify a value for the `max plaintext length` parameter,


Suggested change

the user may optionally specify a value for the `max plaintext length` parameter,

the user MAY optionally specify a value for the `max plaintext length` parameter,

mattsb42-aws · 2020-07-16T21:01:38Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+If the user does not specify a `max plaintext length` value,
+then the caching CMM will neither use cached materials
+nor cache materials from its underlying CMM.


I think we can simplify this to say something along the lines of "the caching CMM will pass through to the underlying CMM without involving the cache".

mattsb42-aws · 2020-07-16T21:02:15Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+So whenever the user can determine an appropriate upper bound for bytes they will encrypt,
+they should specify the `max plaintext length` value.


Suggested change

So whenever the user can determine an appropriate upper bound for bytes they will encrypt,

they should specify the `max plaintext length` value.

If the user can determine an appropriate upper bound for bytes they will encrypt,

they SHOULD specify the `max plaintext length` value.

mattsb42-aws · 2020-07-16T21:08:04Z

changes/2020-07-13_caching-cmm-max-plaintext-length/change.md

+If the user customizes the underlying CMM of a caching CMM,
+then whenever the caching CMM makes a Get Encryption Materials call
+to the customized underlying CMM,
+the Encryption Materials Request will have a `max plaintext length` value
+equal to the `byte limit` value configured on the caching CMM.


What does "customizes the underlying CMM" mean in this case? This statement is true regardless of what underlying CMM you use.

doc: specify how caching CMM should handle max plaintext length in Ge…

7c824aa

…t Encryption Materials

alex-chew requested review from mattsb42-aws and lavaleri and removed request for mattsb42-aws July 15, 2020 16:28

alex-chew force-pushed the caching-cmm-max-plaintext-length branch 2 times, most recently from 43c69f3 to 7c824aa Compare July 15, 2020 16:54

alex-chew added this to the Spec 1.0 (Official Keyring Release) milestone Jul 15, 2020

alex-chew changed the title ~~doc: specify how caching CMM should handle max plaintext length in Get Encryption Materials~~ docs: specify how caching CMM should handle max plaintext length in Get Encryption Materials Jul 15, 2020

lavaleri suggested changes Jul 15, 2020

View reviewed changes

mattsb42-aws assigned alex-chew Jul 15, 2020

acioc reviewed Jul 15, 2020

View reviewed changes

seebees requested changes Jul 16, 2020

View reviewed changes

alex-chew added 2 commits July 16, 2020 11:28

Apply copy edits from code review

524f7a1

Clarify unsafe max plaintext length example

89024b3

mattsb42-aws reviewed Jul 16, 2020

View reviewed changes

alex-chew modified the milestones: Spec 1.0 (Official Keyring Release), Spec 1.1 Jul 16, 2020

Merge branch 'master' into caching-cmm-max-plaintext-length

82ee1e9

Merge branch 'master' into caching-cmm-max-plaintext-length

860226a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: specify how caching CMM should handle max plaintext length in Get Encryption Materials #159

docs: specify how caching CMM should handle max plaintext length in Get Encryption Materials #159

alex-chew commented Jul 15, 2020

lavaleri left a comment

lavaleri Jul 15, 2020

seebees Jul 16, 2020

alex-chew Jul 16, 2020

acioc left a comment

acioc Jul 15, 2020

alex-chew Jul 16, 2020

acioc commented Jul 15, 2020

seebees Jul 16, 2020

mattsb42-aws Jul 16, 2020

mattsb42-aws Jul 16, 2020

mattsb42-aws Jul 16, 2020

mattsb42-aws Jul 16, 2020

mattsb42-aws Jul 16, 2020

mattsb42-aws Jul 16, 2020

		\| Python \| 0.1.0-preview \| 1.3.0 \| [materials_managers/caching.py](https://github.com/aws/aws-encryption-sdk-python/blob/master/src/aws_encryption_sdk/materials_managers/caching.py) \|
		\| Java \| 0.1.0-preview \| 1.3.0 \| [CachingCryptoMaterialsManager.java](https://github.com/aws/aws-encryption-sdk-java/blob/master/src/main/java/com/amazonaws/encryptionsdk/caching/CachingCryptoMaterialsManager.java) \|


		## Security Implications

		This change SHOULD NOT have any security implications.

	the user may optionally specify a value for the `max plaintext length` parameter,
	the user MAY optionally specify a value for the `max plaintext length` parameter,

		So whenever the user can determine an appropriate upper bound for bytes they will encrypt,
		they should specify the `max plaintext length` value.

docs: specify how caching CMM should handle max plaintext length in Get Encryption Materials #159

Are you sure you want to change the base?

docs: specify how caching CMM should handle max plaintext length in Get Encryption Materials #159

Conversation

alex-chew commented Jul 15, 2020

lavaleri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acioc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acioc commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment