Rewrite Keyring interfaces in functional style #56
Conversation
Partial work: still need to rewrite the implementations. Introduces Data Key Materials as the result of Keyring.OnEncrypt as well.
|
Do OnEncrypt and OnDecrypt need to return the same Data Key Materials? OnDecrypt wouldn't populate the Encrypted Data Keys in the response, it doesn't seem ideal to me for the response to contain null values on success. |
| - [Encrypted Data Keys](#structures.md#encrypted-data-keys) | ||
| - Every encrypted data key in this list MUST correspond to the above plaintext data key. | ||
| - [Data Key Materials](#structures.md#data-key-materials.md) | ||
| - If the encryption materials request contains an algorithm suite, the data key materials returned SHOULD contain the same algorithm suite. |
There was a problem hiding this comment.
Why is this a SHOULD instead of a MUST?
There was a problem hiding this comment.
I've heard the CMM interface is permitted to elect to use a different algorithm suite than the one requested. This was implicitly allowed in the stateful version, since there was no requirement that said it couldn't be mutated.
|
|
||
| If the keyring did not attempt any of the above behaviors, it MUST output the | ||
| [encryption materials](#structures.md#encryption-materials) unmodified. | ||
| If the keyring did not attempt any of the above behaviors, it MUST produce no output. |
There was a problem hiding this comment.
Does this language allow for an exception to be thrown if nothing was attempted?
There was a problem hiding this comment.
"The interface MUST fail" is the equivalent language for throwing an exception. I'm using "produce no output" to mean produce null or None or something equivalent rather than a result or Some(something). We need something like this to describe interfaces that are allowed to "do nothing" A keyring can decide to not attempt to decrypt any encrypted data keys, but that's different than attempting and failing.
There was a problem hiding this comment.
What if it threw something like a EncryptionNotRequiredException? Just trying to see if we can avoid returning null. It could return Optional but I'm not sure I love that either
There was a problem hiding this comment.
Nah, not a fan of that since it's encouraging exceptions-as-control-flow. This isn't an exceptional situation so it shouldn't produce an exception. :)
Note that the abstract descriptions of behaviour can map to actual implementation in non-trivial ways. "produce no output" might be "don't populate any of the out parameters" if that is unambiguous (and that's what the older version was dictating in a more restrictive way).
There was a problem hiding this comment.
Nah, not a fan of that since it's encouraging exceptions-as-control-flow. This isn't an exceptional situation so it shouldn't produce an exception. :)
This is a language-specific design choice and as such should not be enshrined in the specification. This is why we used the "must fail" terminology; it leaves the decision to the implementor how specifically to manifest "failure" in their implementation.
Note that the abstract descriptions of behaviour can map to actual implementation in non-trivial ways. "produce no output" might be "don't populate any of the out parameters" if that is unambiguous (and that's what the older version was dictating in a more restrictive way).
We should not be intentionally opening the spec to interpretation in this way. "How do I fail" is a very different thing than "maybe I'll emit something, maybe I won't, maybe that means something, maybe it doesn't".
There was a problem hiding this comment.
Yup, I agree it's a very different thing, and I was describing one option for "produce no output", not "fail".
In functional style this would mean, for example, returning an Option<T>. "producing no output" would map to returning None, but "failing" would make to raising an exception. Or in some cases you would return an Either<Option<T>, Error>, in which case "producing no output" would mean returning Left(None) whereas "failing" would mean returning Right(Error(...)).
I completely agree that we shouldn't dictate how either behaviour is implemented, but they must be implemented as distinguishable behaviours somehow.
|
|
||
| If OnDecrypt fails to successfully decrypt any [encrypted data key](#structures.md#encrypted-data-key), | ||
| then OnDecrypt MUST output the unmodified input [decryption materials](#structures.md#decryption-materials). | ||
| then OnDecrypt MUST produce no output. |
There was a problem hiding this comment.
Similar to OnEncrypt, does this language allow for an exception to be thrown if it wasn't able to successfully decrypt?
There was a problem hiding this comment.
See previous reply :)
| The following inputs are REQUIRED: | 8000||
|
|
||
| - [Algorithm Suite](#algorithm-suites.md) | ||
| - [Encryption Context](#structures.md#encryption-context) | ||
| - [Encrypted Data Keys](#structures.md#encrypted-data-keys) |
There was a problem hiding this comment.
Does this need to be repeated for each Keyring implementation, since the it is specified in the Keyring interface? Maybe somewhere at the top we could just say This keyring MUST implement the Keyring interface with all required inputs and outputs, or something to that effect
There was a problem hiding this comment.
Agreed, there are a few cases with redundant clauses like this but I don't think its that helpful.
|
|
||
| The [algorithm suite](#algorithm-suites.md) to be used for [encryption](#encrypt.md). | ||
| The plaintext data key SHOULD offer an interface to zero the plaintext data key | ||
|
|
There was a problem hiding this comment.
One thing that came up in the review of the Java Keyring was that the object comparison method (equ 8000 als) for plaintext data key should not be vulnerable to timing attacks, ie the byte array should be compared in a time-safe way and not short circuit comparison when a byte differs. You could mention something about that here
There was a problem hiding this comment.
Good point. I cut #57 to track that.
Co-Authored-By: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com>
I'm waffling on this myself - it feels more convenient than correct. :) I might prefer to specify "multiple return values" instead, with phrases like "the Foo interface MUST produce an A and a B" if there's no value in packaging them up into a concept that's nothing more than a result tuple. Many implementations would create a "FooResult" structure in practice to implement that, but some languages will support multiple return values natively and won't need it. |
There was a problem hiding this comment.
+1 on the oddness with OnDecrypt returning a structure where one value will always be empty/null.
I see two main changes in this PR:
- Updating the language so that we speak about inputs/outputs functionally.
- Define a new data key materials structure that the keyrings can return OnDecrypt and OnEncrypt.
There are a couple things about this structure that don't feel quite right to me yet, and I've noted in the PR feedback. For alternatives, we could instead still strictly speak in terms of encryption/decryption materials being inputted/outputted by the keyrings. On the other end we could speak purely in terms of outputs without any particular structure. Or we can try to refine this middle ground some more. With any approach we have to be careful that the properties that we care about on output are preserved (ie the pdks returned match the algorithmSuiteID input, the edks returned match the pdk input, the edks outputted match the pdk outputted).
| - [Plaintext Data Key](#structures.md#plaintext-data-key) | ||
|
|
||
| This interface MAY return [data key materials](#structures.md#data-key-materials) appropriate for the request. If it does return this output | ||
| and a plaintext data key was provided as input, the output must specify the same plaintext data key. |
There was a problem hiding this comment.
Similarly we need to specify in this case that the algorithm suite on output MUST be the same as the one on input.
There was a problem hiding this comment.
And in that case I'd rather not return it at all from Keyring operations since that's simpler. :) I'm starting to lean more towards just returning multiple values rather than a structure.
There was a problem hiding this comment.
That's where I'm leaning too, and in that case we'd need to ensure that we specify that the outputted plaintextDataKey meets the spec for the AlgorithmSuiteID on input, since it would no longer be tied to the plaintextDataKey through the DataKeyMaterials.
|
|
||
| Encryption materials are a structure containing materials needed for [encryption](#encrypt.md). | ||
| This structure MAY include any of the following fields: | ||
| ##### Algorithm Suite |
There was a problem hiding this comment.
Conceptually, it feels like the algorithm suite belongs to the encryption/decryption materials, as it informs that algorithm/parameters being used for encryption/decryption. The only way in which it is concerned with the data key materials is that the data key materials must conform to the parameters defined in the particular algorithm suite.
As such, I think it would be more correct to place the algorithm suite in the encryption/decryption materials, and then have the encryption/decryption materials enforce that the data key materials conform to it's algorithm suite. Thoughts?
There was a problem hiding this comment.
Yeah I agree - I'm thinking more that I would rather express the required relationship between input and output values, rather than duplicating values just so the requirement is only on the output as a separate structure.
|
|
||
| Generate Data Key MAY modify the following fields in the [encryption materials](#structures.md#encryption-materials): | ||
| Generate Data Key MAY populate the following fields in the [data key materials](#structures.md#data-key-materials): |
There was a problem hiding this comment.
It feels unclear whether this is suggesting to create data key materials with no algorithmSuiteID. Data key materials shouldnt be valid without an algorithmSuiteID, however we want to be sure that the one ultimately returned by OnEncrypt matches the one on input.
|
|
||
| - [plaintext data key](#structures.md#plaintext-data-key) | ||
| - [keyring trace](#structures.md#keyring-trace) | ||
|
|
||
| To perform this behavior, the keyring generates a [plaintext data key](#structures.md#plaintext-data-key) | ||
| and sets the resulting plaintext data key on the [encryption materials](#structures.md#encryption-materials). | ||
| and includes the resulting plaintext data key in the [data key materials](#structures.md#data-key-materials)' |
There was a problem hiding this comment.
"in the data key materials returned by OnEncrypt"?
|
|
||
| If the call succeeds, OnEncrypt MUST verify that the response `Plaintext` length matches the specification | ||
| of the [algorithm suite](#algorithm-suites.md). | ||
| If it does not, OnEncrypt MUST fail. | ||
| If verified, OnEncrypt MUST do the following with the response from [KMS GenerateDataKey](#kms-generatedatakey): | ||
|
|
||
| - set the plaintext data key on the [encryption materials](#structures.md#encryption-materials) | ||
| - set the plaintext data key on the [data key materials](#structures.md#data-key-materials) |
There was a problem hiding this comment.
Is it clear enough from context here that we are referring to the data key materials that the KMS Keyring should ultimately output?
|
|
||
| The keyring must use AES-GCM with the following specifics: | ||
|
|
||
| - it uses the [encryption context in the encryption materials](#structures.md#encryption-materials) | ||
| - it uses the specified [encryption context](#structures.md#encryption-context) |
| This record MUST contain this keyring's [key name](#key-name) and [key namespace](#key-namespace), | ||
| and the [flags](#structures.md$flags) field of this record MUST include the following flags: | ||
| - [DECRYPTED DATA KEY](#structures.md#supported-flags) | ||
| - add the resulting plaintext data key to the decryption materials and return the modified materials. | ||
| - specify the resulting plaintext data key in the output [data key materials](#structures.md#data-key-materials). |
There was a problem hiding this comment.
I think I need different language - "set" implies mutating an existing object too strongly, so I worry about confusing the reader. I'm not sure what would be better though. Perhaps "include"?
|
|
||
| The [encryption context](#encryption-context) associated with this [encryption](#encrypt.md). | ||
| An optional [keyring trace](#keyring-trace) containing all of the actions that keyrings have taken on this set of data key materials. |
There was a problem hiding this comment.
This gets a bit wonky when thinking about what things like multikeyrings should return. Technically the multikeyring is the only one interacting with the set of data materials it returns, and it doesnt directly encrypt/decrypt anything, so should it return an empty keyring trace? We should more strongly tie the keyring trace with the plaintext data key and encrypted data keys.
Maybe better wording would be more along the lines of: "all the actions that keyrings have taken related to the pdk and edks in this set of data key materials"
|
|
||
| ##### Keyring Trace | ||
| ### Encryption Materials |
There was a problem hiding this comment.
The lack of symmetry between encryption materials and decryption materials feels a bit off, especially since the decrypt materials path needs to make the translation from data key materials to decryption materials, while the get encryption materials path uses the data key materials directly.
| This interface MAY return [data key materials](#structures.md#data-key-materials) appropriate for the request. If it does return this output | ||
| and a plaintext data key was provided as input, the output must specify the same plaintext data key. |
There was a problem hiding this comment.
on encrypt MUST always emit data key materials. They MAY have additional contents that were not present in the input materials.
Changing to "return nothing means nothing changed" is a fundamental change to the keyring interface and seems to place the onus of consolidating materials on the caller rather than the keyring.
|
|
||
| If the keyring did not attempt any of the above behaviors, it MUST output the | ||
| [encryption materials](#structures.md#encryption-materials) unmodified. | ||
| If the keyring did not attempt any of the above behaviors, it MUST produce no output. |
There was a problem hiding this comment.
Nah, not a fan of that since it's encouraging exceptions-as-control-flow. This isn't an exceptional situation so it shouldn't produce an exception. :)
This is a language-specific design choice and as such should not be enshrined in the specification. This is why we used the "must fail" terminology; it leaves the decision to the implementor how specifically to manifest "failure" in their implementation.
Note that the abstract descriptions of behaviour can map to actual implementation in non-trivial ways. "produce no output" might be "don't populate any of the out parameters" if that is unambiguous (and that's what the older version was dictating in a more restrictive way).
We should not be intentionally opening the spec to interpretation in this way. "How do I fail" is a very different thing than "maybe I'll emit something, maybe I won't, maybe that means something, maybe it doesn't".
| and includes the resulting plaintext data key in the [data key materials](#structures.md#data-key-materials)' | ||
| [data key materials](#structures.md#data-key-materials). |
There was a problem hiding this comment.
"includes the data key in the data key materials' data key materials"???
I have no idea what that is supposed to mean...do the data key materials separately contain data key materials?? At best that is confusing.
There was a problem hiding this comment.
Stupid typo, mea cupla. Fixed.
|
|
||
| #### Structure | ||
| TODO: The above is only a similar data structure related to the deprecated master key/master key provider concepts. |
There was a problem hiding this comment.
then don't list it as an implementation
There was a problem hiding this comment.
Yeah agreed - I'm going to describe how the existing implementations inline the structure instead.
|
|
||
| - fit the specification for the [key derivation algorithm](#algorithm-suites.md#key-derivation-algorithm) | ||
| included in this decryption material's [algorithm suite](#algorithm-suite). | ||
| - consist of cryptographically secure (pseudo-)random bits. |
There was a problem hiding this comment.
I specifically remember a lengthy discussion around wording regarding the entropy of the plaintext data key. There should be a definition elsewhere in this spec.
There was a problem hiding this comment.
Is the language in https://github.com/awslabs/aws-encryption-sdk-specification/blob/master/data-format/message-header.md#message-id what you are referring to?
10000
framework/structures.md
|
|
||
| The [encryption context](#encryption-context) associated with this [encryption](#encrypt.md). | ||
| An optional [keyring trace](#keyring-trace) containing all of the actions that keyrings have taken on this set of data key materials. |
There was a problem hiding this comment.
The keyring trace is not optional.
|
|
||
| Encrypt Data Key MAY modify the following fields in the [encryption materials](#structures.md#encryption-materials): | ||
| Encrypt Data Key MAY populate the following fields in the [data key materials](#structures.md#data-key-materials): |
There was a problem hiding this comment.
"populate" implies that they are empty. What about keyrings that receive input that already contains EDKs? How about "add to"?
| If OnEncrypt outputs [data key materials](#structures.md#data-key-materials) with at least | ||
| one [encrypted data key](#structures.md#encrypted-data-key), | ||
| the [keyring trace](#structures.md#keyring-trace) returned by OnEncrypt MUST include at least one trace |
There was a problem hiding this comment.
This wording changes the intent, back to my point about return values and data key material composition.
| If this keyring attempted the above behavior, and succeeded, it MUST output the resulting [data key materials](#structures.md#data-key-materials). The [algorithm suite](#algorithm-suites.md) in the result must match the input [algorithm suite](#algorithm-suites.md), and the [encrypted data keys](#structures.md#encrypted-data-keys) | ||
| list must be empty. |
There was a problem hiding this comment.
and the encrypted data keys list must be empty
What? This sounds like you are saying that a keyring would modify the EDK list. That would be a HUGE breaking change and I haven't seen any justification for it...
|
|
||
| Otherwise, OnDecrypt MUST attempt to decrypt the [encrypted data keys](#structures.md#encrypted-data-key) | ||
| in the input [decryption materials](#structures.md#decryption-materials) using it's | ||
| OnDecrypt MUST attempt to decrypt the inputted [encrypted data keys](#structures.md#encrypted-data-key) using its |
There was a problem hiding this comment.
There is a lot of intent lost here. Why?
|
I realized this morning that I don't think I ever summarized my issues with this change. Rather than simply modifying the promises around keyring side effects, by stating that "
This is a significant change from the current statement that " The change as proposed also removes the binding between the various components of the encryption/decryption materials. If the main thing you are after is the removal of side effects, we can do that without fundamentally changing the interface and boundaries of responsibility. However, I think that making even that a MUST rather than a SHOULD requires significant justification because it might not always be viable for every language. |
|
@mattsb42-aws and I discussed this PR offline, and I'm planning to cut a separate issue along the lines of "describe implementations such that pure functional implementations are allowed". This will help describe the motivation and more clearly describe what the actual changes are (since the diff is just awful to try to understand by itself). We should be able to avoid describing things in terms of mutation, but I will likely want to be less minimalistic about describing inputs and outputs: there are advantages to keeping related data together in |
|
Separate issue cut on this topic: #66 Closing this and will revisit at a later date. |
Adding AWS KMS RSA Asymmetric key support
Adding AWS KMS RSA Asymmetric key support
#53
The current specification defines the behaviour of the
Keyringconcept in terms of mutation side-effects on theEncryptionMaterialsandDecryptionMaterialsstructures. This change refactors theKeyringinterfaces to specify separate immutable input and output structures.In order to do so, it introduces a new
Data Key Materialsconcept that acts as the return value fromKeyring.OnEncryptandKeyring.OnDecrypt.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.