chore(decrypt): specify behavior on decrypt with encryption context #261

josecorella · 2023-09-27T23:22:40Z

Issue #, if available:

Description of changes:

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

Check any applicable:

Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

changes/2023-09-26_encryption-context-decrypt/background.md

lavaleri · 2023-09-29T18:05:33Z

changes/2023-09-26_encryption-context-decrypt/background.md

+It MUST be required that the decryption materials obtained from the underlying CMM's MUST
+contain all configured encryption context keys in its encryption context and the values MUST remain
+unchanged.


I'm not sure what this means. What is a "configured encryption conext key? How do we enforce they remain unchanged?

"configured encryption conext key" this should be "required encryption context key"

I am trying to get at is that if the CMM returns encryption context which IS stored AND one of the keys in the required encryption context is also in this encryption context then the value in their key-value pair mapping MUST remain unchanged

If we break down what's in the PR:

contain all required encryption context key in (the decryption material's) encryption context

This is already a requirement on decryption materials. If a "key" is required, that key-value pair must exist in the encryption context on those materials.

the values (of the required encryption context keys? or all EC values?) MUST remain unchanged (from what?)

Which values? (it should be the values corresponding to the required encryption context keys)

What does "unchanged" mean? Unchanged from what? (Unchanged from the EC in the header, if that key also exists in the header)

Suggested change

It MUST be required that the decryption materials obtained from the underlying CMM's MUST

contain all configured encryption context keys in its encryption context and the values MUST remain

unchanged.

The decryption materials obtained from the underlying CMM's MUST

contain all required encryption context keys in its encryption context and the values MUST remain

unchanged.

Is the problem that https://github.com/awslabs/aws-encryption-sdk-specification/blob/master/framework/cmm-interface.md#decrypt-materials

The CMM MUST validate the Encryption Context by comparing it to the customer supplied Reproduced Encryption Context in decrypt materials request. For every key that exists in both Reproduced Encryption Context and Encryption Context, the values MUST be equal or the operation MUST fail.

is not a post condition?

lavaleri · 2023-09-29T21:40:58Z

changes/2023-09-26_encryption-context-decrypt/background.md

+### Test Different Encryption Context on Decrypt Failure
+
+Encrypt with and store all encryption context in the message header.
+On Decrypt will supply additional encryption context not stored in the header.


This is testing a total disjoint between expected EC and inputted EC. We should seperately test "inputted EC is missing a pair", "inputted EC has an incorrect value for one pair", and "inputted EC has an extra pair".

Looking at the tests below, I think we cover all of these except "inputted EC has an extra pair"

adding in latest revision

As set up, this could fail due to either:

reproduced EC missing required key-value pairs

reproduced EC including an extra key-value pair

Which one do we want this test to target?

lavaleri · 2023-09-29T21:42:36Z

changes/2023-09-26_encryption-context-decrypt/background.md

+// Test supply same encryption context on encrypt and decrypt NO filtering
+var encryptionContext := map[ keyA := valA, keyB := valB ];
+var reproducedAdditionalEncryptionContext := map[ keyA := valB, keyB := valA  ];
+var requiredEncryptionContextKeys := [keyA, keyB];


I thought this was testing context that is in the header?

yup good catch

changes/2023-09-26_encryption-context-decrypt/change.md

lavaleri · 2023-09-29T21:51:51Z

changes/2023-09-26_encryption-context-decrypt/change.md

+NOT some key-value pair that the underlying CMM MAY have modified.
+
+If required keys exist, match stored encryption context, AND decryption succeeds
+Decrypt MUST return both the Encryption Context stored in the header AND the


This "AND" is ambiguous. I believe it should be the union of these maps.

Actually, should it just replicate our logic for inclusion in the header validation?
i.e. Every key-value pair in the header, and every "required" key-value pair in the EC?

client-apis/decrypt.md

Co-authored-by: lavaleri <[email protected]>

lavaleri · 2023-10-03T16:52:40Z

changes/2023-09-26_encryption-context-decrypt/background.md

+Decrypt returns Encryption Context.
+This requirement may be satisfied by returning the parsed header that contains this value.
+
+With the addition of the [required encryption context cmm](../../framework/required-encryption-context-cmm.md),
+the encrypt API is able to filter out encryption context key-value pairs that
+are not stored on the message.


nit: I'd flip the order of these and edit it to say that we used to be able to satisfy the requirement by just returning the header, but that is no longer the case, as the header may not contain all EC values used to authenticate the message anymore.

changes/2023-09-26_encryption-context-decrypt/background.md

lavaleri · 2023-10-03T17:06:48Z

changes/2023-09-26_encryption-context-decrypt/background.md

+It MUST be required that the decryption materials obtained from the underlying CMM's MUST
+contain all configured encryption context keys in its encryption context and the values MUST remain
+unchanged.


If we break down what's in the PR:

contain all required encryption context key in (the decryption material's) encryption context

This is already a requirement on decryption materials. If a "key" is required, that key-value pair must exist in the encryption context on those materials.

the values (of the required encryption context keys? or all EC values?) MUST remain unchanged (from what?)

Which values? (it should be the values corresponding to the required encryption context keys)

What does "unchanged" mean? Unchanged from what? (Unchanged from the EC in the header, if that key also exists in the header)

lavaleri · 2023-10-03T18:39:57Z

changes/2023-09-26_encryption-context-decrypt/background.md

+
+On Encrypt will write all encryption context supplied to the message.
+On Decrypt will have a Required Encryption Context CMM that requires all encryption context
+on Decrypt.


This MUST succeed.

lavaleri · 2023-10-03T18:44:16Z

changes/2023-09-26_encryption-context-decrypt/background.md

+### Test Different Encryption Context on Decrypt Failure
+
+Encrypt with and store all encryption context in the message header.
+On Decrypt will supply additional encryption context not stored in the header.


As set up, this could fail due to either:

reproduced EC missing required key-value pairs

reproduced EC including an extra key-value pair

Which one do we want this test to target?

lavaleri · 2023-10-03T18:45:10Z

changes/2023-09-26_encryption-context-decrypt/background.md

+
+// Test supply same encryption context on encrypt and decrypt NO filtering
+var encryptionContext := map[ keyA := valA, keyB := valB ];
+var reproducedAdditionalEncryptionContext := map[ keyA := valA, keyB := valB, keyC := valC ];


To test mismatch, don't you want this to be something like map[ keyA := valA, keyB := valC ];? i.e. the keys are correct but the values are not? Otherwise this is testing "extra unexpected key-value pair" again.

lavaleri · 2023-10-03T18:46:35Z

changes/2023-09-26_encryption-context-decrypt/background.md

+
+Encrypt will NOT store all encryption context.
+Decrypt will NOT supply reproduced encryption context.
+This operation MUST fail.


Is this failing due to the Required Encryption Context CMM, or is authentication failing? Which are we trying to put under test? These descriptions should make clear whether or not the REC CMM is involved or not.

lavaleri · 2023-10-03T19:09:16Z

changes/2023-09-26_encryption-context-decrypt/background.md

+   :: decryptionMaterials[k] == headerEncryptionContext[k]
+```
+
+## Testing


Thinking more about this, I think it would be better to more clearly define what the matrix is we care about, as opposed to listing out the different tests and code for them. I would prefer we cut the example code, but make it extremely clear what every case we need to check is in a way that makes it easy to reason that it is complete. For example, maybe frame it like this:

First, suppose we have a message where {A:B,C:D} is in the header, and {E:F} is not stored, but is included in the authentication.

For this message, go through the following test cases:

customer supplies correct required key-value {E:F} (SUCCESS)

customer supplies correct required key-value and additional correct header value {A:B,E:F} (SUCCESS)

customer supplies does not supply required key-value {} (FAILURE)

customer supplies incorrect value for required key-value {E:X} (FAILURE)

customer supplies correct required key-value and additional incorrect header value {A:X,E:F} (FAILURE)

customer supplies correct required key-value and additional incorrect, non-header key-value {E:F,X:Y} (FAILURE)

For every one of the above cases, attempt to decrypt using both a Default CMM, and a REC CMM with E configured as a "required key".

Next, suppose we have a message where {A:B,C:D,E:F} is in the header, and there is no additional non-stored key-values authenticated with the message.

For this message, go through the following test cases:

customer supplies no reproduced context {} (SUCCESS)

customer supplies additional correct header values {A:B,C:D,E:F} (SUCCESS)

customer supplies incorrect value for key in header {A:X} (FAILURE)

customer supplies incorrect, non-header key-value {X:Y} (FAILURE)

For each of these use a Default CMM.

This is more test cases than we currently have, and we may be able to convince ourselves that some of those test cases are not actually useful. However, I would prefer if this document focused on listing the parameters for the test cases in a way that is easy for readers to see whether we are missing any (And if we decide any are not useful, say why). If we do this, I don't think we need to give example code for each test case.

lavaleri · 2023-10-03T19:15:25Z

client-apis/decrypt.md

@@ -162,6 +164,8 @@ that implementation SHOULD NOT provide an API that allows the caller to stream t

 The [encryption context](../framework/structures.md#encryption-context) that is used as
 additional authenticated data during the decryption of the input [encrypted message](#encrypted-message).
+Specifically, it MUST be the union of the encryption context serialized into the message header and
+the encryption context for authentication only, if available.


encryption context for authentication only Can we point to the definition of this somewhere?

changes/2023-09-26_encryption-context-decrypt/background.md

seebees · 2023-10-03T17:16:22Z

changes/2023-09-26_encryption-context-decrypt/background.md

+It MUST be required that the decryption materials obtained from the underlying CMM's MUST
+contain all configured encryption context keys in its encryption context and the values MUST remain
+unchanged.


Suggested change

It MUST be required that the decryption materials obtained from the underlying CMM's MUST

contain all configured encryption context keys in its encryption context and the values MUST remain

unchanged.

The decryption materials obtained from the underlying CMM's MUST

contain all required encryption context keys in its encryption context and the values MUST remain

unchanged.

Is the problem that https://github.com/awslabs/aws-encryption-sdk-specification/blob/master/framework/cmm-interface.md#decrypt-materials

The CMM MUST validate the Encryption Context by comparing it to the customer supplied Reproduced Encryption Context in decrypt materials request. For every key that exists in both Reproduced Encryption Context and Encryption Context, the values MUST be equal or the operation MUST fail.

is not a post condition?

seebees · 2023-10-03T17:36:42Z

changes/2023-09-26_encryption-context-decrypt/background.md

+The Decrypt API MUST return the set of the union of the encryption context stored in the header and the encryption
+context used for authentication only.


I agree but this feels like a jump.

seebees · 2023-10-03T17:45:25Z

changes/2023-09-26_encryption-context-decrypt/background.md

+The Decrypt API MUST return the set of the union of the encryption context stored in the header and the encryption
+context used for authentication only.
+
+### What changes to the existing Required Encryption Context CMM are required?


Are there any changes to the CMM interface?

changes/2023-09-26_encryption-context-decrypt/background.md

seebees · 2023-10-03T20:17:11Z

changes/2023-09-26_encryption-context-decrypt/change.md

+Decrypt MUST verify that if the keys in the required encryption context keys exist in the reproduced encryption context
+AND they are stored in the message header, they MUST have the same value as the reproduced encryption context supplied.


This is not how you have change the spec:

- If a [required encryption context key](../framework/structures.md#required-encryption-context-keys) in the decryption materials' response exists in the [message header body](../data-format/message-header.md#header-body) the value in the decryption materials' encryption context MUST equal the value stored in the message header body.

If you want what you have specified, then I think you need:

Suggested change

Decrypt MUST verify that if the keys in the required encryption context keys exist in the reproduced encryption context

AND they are stored in the message header, they MUST have the same value as the reproduced encryption context supplied.

For required encryption context keys the value in stored in the message header

MUST equal the value in decryption materials encryption context.

seebees · 2023-10-05T21:58:14Z

changes/2023-09-26_encryption-context-decrypt/background.md

+A poorly designed CMM could theoretically return materials with an "incorrect" encryption context
+that contradicts the encryption context in the header of the message.
+
+The `decrypt` API SHOULD not check if the encryption context that was stored on the message matches


Suggested change

The `decrypt` API SHOULD not check if the encryption context that was stored on the message matches

The `decrypt` API SHOULD NOT check if the encryption context that was stored on the message matches

seebees · 2023-10-05T21:59:27Z

changes/2023-09-26_encryption-context-decrypt/background.md

+
+The `decrypt` API SHOULD not check if the encryption context that was stored on the message matches
+what it receives from the CMMs DecryptMaterials because the current implementation
+of the `encrypt` API used with the built-in CMMs is not able to write a message


Stronger than built-in.
It constructs the 2 sets of EC from a single set.
So these 2 sets (stored/not stored) MUST be disjoint.

seebees · 2023-10-05T22:00:37Z

changes/2023-09-26_encryption-context-decrypt/background.md

+- `{ b: b, c: c}`
+- `{ a: a, b: b, c: c }`
+
+### Message: `{a: a, b: b}` / `{}`


I think we can further wordsmith these tables to simplify.
But this is a good start!

lavaleri · 2023-10-05T21:27:47Z

changes/2023-09-26_encryption-context-decrypt/background.md

+- Stored EC -- EC stored in the header
+- Not stored EC -- EC authenticated to the header but not stored
+- CMM Material EC -- EC on the decryption material
+- Required keys -- Keys for the EC that MAY be only authenticated


As described by the CMM in the decryption materials? Otherwise this is "the same" as "Not stored EC"

lavaleri · 2023-10-05T21:28:30Z

changes/2023-09-26_encryption-context-decrypt/background.md

+- `{a: a, b: b}` / `{a}`
+- `{a: a, b: b}` / `{a,b}`
+- `{a: a, b: c}` / `{a}`
+- `{a: a, b: b}` / `{c}`


The specification for decryption materials doesn't allow this case.

lavaleri · 2023-10-05T21:37:44Z

changes/2023-09-26_encryption-context-decrypt/background.md

+| CMM Material/Required Keys &rarr; <br/>Reproduced EC &darr; | `{a: a, b: b}` / `{}` | `{a: a, b: b}` / `{a}` | `{a: a, b: b}` / `{a,b}` | `{a: a, b: c}` / `{a}` | `{a: a, b: b}` / `{c}` |
+| ----------------------------------------------------------- | --------------------- | ---------------------- | ------------------------ | ---------------------- | ---------------------- |
+| `{}`                                                        | pass                  | fail                   | fail                     | fail                   | fail                   |
+| `{ a: a }`                                                  | pass                  | pass                   | fail                     | fail                   | fail                   |


We should expect every column except for the first to always fail during the header authentication step, regardless of the inputted reproduced encryption context. ('a' or 'b' would be incorrectly appended to the header authentication).

lavaleri · 2023-10-05T21:43:50Z

client-apis/decrypt.md

+Specifically, it MUST be the union of the encryption context serialized into the message header and
+the [encryption context for authentication only](#encryption-context-to-only-authenticate), if available.


We know that by the time you are returning a result that these maps are disjoint, but this won't be something we will be able to prove in dafny... I'm unsure whether we want to specifically make a note in the spec of this expectation or not.

lavaleri · 2023-10-05T21:48:25Z

changes/2023-09-26_encryption-context-decrypt/background.md

+
+### Message: `{a: a}` / `{b: b}`
+
+| CMM Material/Required Keys &rarr; <br/>Reproduced EC &darr; | `{a: a, b: b}` / `{}` | `{a: a, b: b}` / `{a}` | `{a: a, b: b}` / `{a,b}` | `{a: a, b: c}` / `{a}` | `{a: a, b: b}` / `{c}` |


I think you actually want the message to be {b: b} /{a: a} Otherwise, all of these columns should always fail, as the header auth won't ever correctly append a 'b' value during verification, regardless of the reproduced encryption context.

lavaleri · 2023-10-05T21:54:17Z

changes/2023-09-26_encryption-context-decrypt/background.md

+
+| CMM Material/Required Keys &rarr; <br/>Reproduced EC &darr; | `{a: a, b: b}` / `{}` | `{a: a, b: b}` / `{a}` | `{a: a, b: b}` / `{a,b}` | `{a: a, b: c}` / `{a}` | `{a: a, b: b}` / `{c}` |
+| ----------------------------------------------------------- | --------------------- | ---------------------- | ------------------------ | ---------------------- | ---------------------- |
+| `{}`                                                        | fail                  | fail                   | fail                     | fail                   | fail                   |


We may want to represent some of these as N/As. For example, if the reproduced context does not contain b, how would the CMM ever be able to return a 'b'.

It may be easier to reason about this matrix if we represent the obviously wrong values with something like x, to better indicate that this is just some junk value, and is possible for the CMM to add to the EC.

lavaleri · 2023-10-05T21:56:43Z

changes/2023-09-26_encryption-context-decrypt/background.md

+
+### Message:`{}` / `{a: a, b: b}`
+
+| CMM Material/Required Keys &rarr; <br/>Reproduced EC &darr; | `{a: a, b: b}` / `{}` | `{a: a, b: b}` / `{a}` | `{a: a, b: b}` / `{a,b}` | `{a: a, b: c}` / `{a}` | `{a: a, b: b}` / `{c}` |


Similar to the other tables, I think that {a: a, b: b} / {} is the only column that could ever succeed.

lavaleri · 2023-10-05T22:01:30Z

changes/2023-09-26_encryption-context-decrypt/background.md

+- `{a: a}` / `{b: b}`
+- `{}` / `{a: a, b: b}`
+
+CMM Material/Required Keys


This is missing:

{a: a, b: b, x: x} / {a}

{a: x, b: b} / {a}

chore(decrypt): specify behavior on decrypt with encryption context

8ba515c

josecorella requested a review from a team as a code owner September 27, 2023 23:22

lavaleri suggested changes Sep 29, 2023

View reviewed changes

josecorella and others added 2 commits October 2, 2023 14:05

Apply suggestions from code review

f164be5

Co-authored-by: lavaleri <[email protected]>

update

9b8233a

lavaleri suggested changes Oct 3, 2023

View reviewed changes

seebees reviewed Oct 3, 2023

View reviewed changes

josecorella added 4 commits October 4, 2023 12:34

update

7dd4dc8

format

2d83dfc

update spec

d257aee

update matrix

656459a

josecorella force-pushed the jocorell/specify-ec-decrypt branch from da25ba4 to 656459a Compare October 5, 2023 21:15

josecorella and others added 2 commits October 5, 2023 14:16

format

c9eae97

Merge branch 'master' into jocorell/specify-ec-decrypt

b32c15f

seebees reviewed Oct 5, 2023

View reviewed changes

lavaleri reviewed Oct 5, 2023

View reviewed changes

		The Decrypt API MUST return the set of the union of the encryption context stored in the header and the encryption
		context used for authentication only.

		Decrypt MUST verify that if the keys in the required encryption context keys exist in the reproduced encryption context
		AND they are stored in the message header, they MUST have the same value as the reproduced encryption context supplied.

	The `decrypt` API SHOULD not check if the encryption context that was stored on the message matches
	The `decrypt` API SHOULD NOT check if the encryption context that was stored on the message matches

		Specifically, it MUST be the union of the encryption context serialized into the message header and
		the [encryption context for authentication only](#encryption-context-to-only-authenticate), if available.


		### Message: `{a: a}` / `{b: b}`

		\| CMM Material/Required Keys → <br/>Reproduced EC ↓ \| `{a: a, b: b}` / `{}` \| `{a: a, b: b}` / `{a}` \| `{a: a, b: b}` / `{a,b}` \| `{a: a, b: c}` / `{a}` \| `{a: a, b: b}` / `{c}` \|


		### Message:`{}` / `{a: a, b: b}`

		\| CMM Material/Required Keys → <br/>Reproduced EC ↓ \| `{a: a, b: b}` / `{}` \| `{a: a, b: b}` / `{a}` \| `{a: a, b: b}` / `{a,b}` \| `{a: a, b: c}` / `{a}` \| `{a: a, b: b}` / `{c}` \|

chore(decrypt): specify behavior on decrypt with encryption context #261

Are you sure you want to change the base?

chore(decrypt): specify behavior on decrypt with encryption context #261

Conversation

josecorella commented Sep 27, 2023

Check any applicable:

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

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

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

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

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

Choose a reason for hiding this comment