Open robin-aws opened 4 years ago
We've all discussed this offline at various points, but just to get it on the record (since there is a record now).
I absolutely agree that the decrypt API should provide a mechanism to check the encryption context before decryption. However, I think that we need to be very careful with whatever we propose to make sure that it covers the use-cases we need before moving on to proposing a specific feature. Whatever we do here becomes a permanent aspect of our API. We need to make sure that we are doing the right thing.
Couldn't agree more - really wanted to at least get the explicit discussion started. :)
I'm also aware that "matching the EC provided to Encrypt
" might be too strong, but I wanted to start there and be argued down if necessary.
"matching the EC provided to Encrypt" might be too strong
Yup. We cannot require an exact match because CMMs can (and do, in the case of the DefaultCMM) add things to the encryption context.
Some patterns that I've heard proposed over the years include:
For completeness, some things that I argue we MUST NOT support:
"matching the EC provided to Encrypt" might be too strong
Yup. We cannot require an exact match because CMMs can (and do, in the case of the DefaultCMM) add things to the encryption context.
Is there any hope of being able to identify what things were added? That would make it possible to require an exact match to what was provided to Encrypt
, if not to what was provided to the keyring(s). I'm not hopeful because I gather we can't assume the decrypting CMM has all the context the encrypting CMM does, and therefore can't know what keys were added.
Some patterns that I've heard proposed over the years include: ...
I'm tempted to just support the first option (key:value pairs that MUST be present) for simplicity (since that just means accepting a map), and say that any more complicated verification could be done afterwards on the Decrypt result. I do realize that means those checks are then weaker since they violate the spirit of #136 (i.e. plaintext will be released before the Decrypt caller gets the chance to validate those properties. I also agree that handling all possible verification isn't feasible though, so we'll have to draw the line somewhere.
Is there any hope of being able to identify what things were added?
It's not impossible, but it would require adding some sort of metadata to the message, especially if we want this check to happen in the client before it calls the CMM.
I'm tempted to just support the first option (key:value pairs that MUST be present) for simplicity (since that just means accepting a map), and say that any more complicated verification could be done afterwards on the Decrypt result.
My concern with that approach is that unless we can make a strong case for why we should not support anything else, that we would just be painting ourselves further into this corner, leading to a further-expanding API in the future.
One idea that I've tossed around with @seebees in the past is a "decryption requirements" structure that could accept various configurations. That would limit the affect to the top-level API while also providing a bounded location to add additional checks in the future. Other things that might make sense for this structure include the maxBodySize limits #72. We would need to be very careful to make sure that this structure is for operational controls only, as security controls MUST happen in the keyring or CMM.
Is there any hope of being able to identify what things were added?
It is possible to determine what was added by comparing the EC passed to the CMM with the EC returned from the CMM. If the NONE of the encryption context(EC) provided on encrypt is stored with the message, then this information is transmitted, by construction, to the Decrypter.
Not storing anything disrupts existing use cases and significantly changes the ESDK interface/contract.
However having values that MUST be provided on decrypt because they are not stored significantly increases the safety of the API. Using unvalidated encryption context is a case of "easy to misuse".
This conversation significantly bleeds into the requirements
set by the encryptor on encrypt
.
As well as changing, and maybe clarifying what a CMM does?
This would make it so that EVERYTHING
that is stored with the message is controlled by the CMM.
I'm tempted to just support the first option (key:value pairs that MUST be present) for simplicity
This is where I land as well. As you say, every possible verification isn't feasible. Furthermore all of these other cases rely on knowing something but not everything about the values in question.
Therefore these values can not be recreated on decrypt
and MUST exist in the message.
In which case they can be checked
by interrogating the header
BEFORE the decrypt
call.
This separates things nicely by structuring this check as a guard. Any action taken at this point can not rely on the encrypted data so it is clear it could not rely on validated data.
Hmm, so a control something along the lines of "do not store caller-provided encryption context" on encrypt and "additional encryption context" on decrypt?
In which case they can be checked by interrogating the header BEFORE the decrypt call.
This still feels like continuing to side-step the problem to me.
In the ESDK the
Decrypt
operation does not offer a way to validate the encryption context in any way. It does return access to the validated encryption context. We tell people that they MUST verify the encryption context, but do not provide good guidance on what exactly this means. This leaves the work to the customer and abdicates our responsibility. Mechanisms trump good intentions, and the current API is a triumph of good intentions.Our examples always demonstrate this verification, but since they often demonstrate the encryption and decryption process within the same code context, it doesn't help illustrate whether the decryptor needs to provide all or just a subset of the encryption context.
We should at a minimum accept the expected encryption context as input to
Decrypt
and verify that it matches the encryption context provided toEncrypt
.(credit to @seebees for some of these words)