feat: remove keyring trace #105
Conversation
| @@ -0,0 +1,186 @@ | |||
| # Remove Keyring Trace | |||
There was a problem hiding this comment.
(file-level comment) Is 2020-05-13_remove-keyring-trace the identifier for this atomic change then? I like being able to include more than one file. Can we structure things so a change is a directory to make that a little cleaner?
There was a problem hiding this comment.
Agreed. I'll move these to a directory structure.
| @@ -0,0 +1,186 @@ | |||
| # Remove Keyring Trace | |||
|
|
|||
| ## Affected Features | |||
There was a problem hiding this comment.
For both "Affected Things" tables: is this worth the trouble to layout when it should be clear from (and consistent with) the set of files the change touches? We can just have a slot for related issues to cover the link to #40.
There was a problem hiding this comment.
I don't think that is sufficient, no. In addition to providing context for the PR, the list of affected things is a historical artifact.
| | C | 0.1.0 | n/a | [aws-encryption-sdk-c](https://github.com/aws/aws-encryption-sdk-c) | | ||
| | Javascript | 0.1.0 | n/a | [aws-encryption-sdk-javascript](https://github.com/aws/aws-encryption-sdk-javascript) | |
There was a problem hiding this comment.
Will the process be:
- Get approval on the PR as usual, and then
- Cut issues on each affected repo immediately before actually merging? Immediately after?
There was a problem hiding this comment.
What I had in mind for this is:
- Change is approved and merged.
- Implementations are updated.
- Change doc is updated with "Version Removed" for each implementation.
I would not expect this section to be present for all spec change docs, but I think that we need this for this one because it is removing a feature, and that removal will not necessarily be obvious if you, say, just look at the feature doc.
There was a problem hiding this comment.
Now that this is spelled out with version bumps, I think that's better. There's no implied race between the spec and the implementations, we just need a good way to track the gap between the current version of the spec and the latest implemented version for each language.
| | Language | Confirmed Compatible with Spec Version | Minimum Version Confirmed | Implementation | | ||
| |----------|----------------------------------------|---------------------------|----------------| | ||
| | C | 0.1.0-preview | 0.1.0 | [materials.h](https://github.com/aws/aws-encryption-sdk-c/blob/master/include/aws/cryptosdk/materials.h) | | ||
| | Javascript | 0.1.0-preview | 0.1.0 | [cryptographic_material.ts](https://github.com/awslabs/aws-encryption-sdk-javascript/blob/master/modules/material-management/src/cryptographic_material.ts)| | ||
| | Python | 0.1.0-preview | n/a | [materials_managers](https://github.com/aws/aws-encryption-sdk-python/blob/master/src/aws_encryption_sdk/materials_managers/__init__.py) | | ||
| | Java | 0.1.0-preview | n/a | [EncryptionMaterials.java](https://github.com/aws/aws-encryption-sdk-java/blob/master/src/main/java/com/amazonaws/encryptionsdk/model/EncryptionMaterials.java) | |
There was a problem hiding this comment.
LOVE having this codified in a table, but I think this will quickly get much more complicated as we have multiple versions in flight. Would it make more sense to have a separate table for each implementation, so that we can have a row for each spec version and the corresponding minimum implementation version?
I realize this is a multi-dimensional matrix and I'm not sure what the optimal rendering is off-hand :)
There was a problem hiding this comment.
My intention is that the spec feature doc always serves as the source of truth for that is true right now. I like the idea of a historical "implementation X complied with feature version X from implementation versions from version M to version N" for every implementation and feature version, but that feels to me like something that would be better represented in a machine-readable form. Say, a separate JSON file.
resolves: aws#152, aws#31 linked: awslabs/aws-encryption-sdk-specification#105 If no keyrings attempt to decrypt any encrypted data keys, then the message can not be decrypted. The code attempted to enforce this, by retrieving the unencrypted data key in node. There were two issues here 1. The check ensure the validity of the materials, itself threw an error. 1. Had this check succeeded, the error message `'Unencrypted data key is invalid.’` is not incredibly more helpful than 'unencryptedDataKey has not been set' The error message has been updated, and the tests have been updated to verify _this_ error message. On a related note awslabs/aws-encryption-sdk-specification#97 starts to explore some additional possibilities. The fullness of this issue is not only in failure, but success can also have similar issues.
Primary Issue
resolves: #97
resolves: #95
resolves: #70
resolves: #78
resolves: #39
resolves: #32
resolves: aws/aws-encryption-sdk-javascript#18
resolves: aws/aws-encryption-sdk-python#181
Summary
We will remove the keyring trace from the AWS Encryption SDK specification
and affected implementations
because we have determined that existing and better-defined parts of
the AWS Encryption SDK framework provide better solutions
to the problems that we intended the keyring trace to solve.
Motivation
We added the keyring trace with the anticipation that it would be a useful tool
to make assertions about what keyrings did to encryption and decryption materials.
However, we never defined how callers should interact with the keyring trace.
Before adding keyrings to additional implementations beyond C and Javascript,
we re-evaluated how callers should interact with the keyring trace
and came to the conclusion that they should not.
We determined that the keyring trace is unnecessary
because all expected use-cases are better solved either
by making keyrings that are correct by construction
or by proactively checking requirements before invoking keyrings.
We had considered adding failure information to the keyring trace,
but upon reviewing the capabilities that we would want in
a tool to communicate failure information,
we came to the conclusion that the keyring trace does not meet those requirements
and that a purpose-built solution will solve that problem better
than retrofitting failure information onto the keyring trace.
Out of Scope
The design for keyring failure communication is out of scope.
That feature is tracked separately.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.