fix: Encryption Context changes #148

seebees · 2019-07-15T17:07:13Z

resolves #54

browser/node encrypt function public api should match
Encryption context should only be optional in the public interface
Encryption context moved into cryptographic materials

Internally, it is much more consistent to require an encryption context.
Moving the encryption context onto the cryptographic materials
brings them in line with the python keyring implementation.
It also makes control of the encryption context in the CMM
and not the keyrings very clear in the interface.

NOTE: encrypt in encrypt-node now expects encryptionContext instead of context.
This is ONLY done because the project is still in beta.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

resolves aws#54 1. browser/node encrypt function public api should match 1. Encryption context should _only_ be optional in the public interface 1. Encryption context moved into cryptographic materials Internally, it is much more consistent to require an encryption context. Moving the encryption context onto the cryptographic materials brings them in line with the python keyring implementation. It also makes control of the encryption context in the CMM and **not** the keyrings very clear in the interface. NOTE: encrypt in encrypt-node now expects `encryptionContext` instead of `context`. This is **ONLY** done because the project is still in beta.

modules/cache-material/test/clone_cryptographic_material.test.ts

lavaleri · 2019-07-16T18:02:32Z

modules/material-management-browser/test/browser_cryptographic_materials_manager.test.ts

-    if (!test.material.signatureKey) throw new Error('I should never see this error')
-    expect(test.context).to.have.haveOwnProperty(ENCODED_SIGNER_KEY).and.to.equal(toBase64(test.material.signatureKey.compressPoint))
-    expect(test.context).to.have.haveOwnProperty('some').and.to.equal('context')
+    const { material } = await cmm.getEncryptionMaterials({ suite, encryptionContext })


My position is that CMMs' getEncryptionMaterials should return encryption materials, not an object containing encryption materials. I know that makes it a bit difficult if we wanted to add something else in to this response later, but I would argue that as long as this function is named correctly, anything that's worth adding to the getEncryptionMaterials response should be added to the encryption materials.

I know that makes it a bit difficult if we wanted to add something else in to this response later

For the record, this is exactly what the encryption materials structure is for. ;) That's what we need to modify if we want to return more things.

lavaleri · 2019-07-16T18:22:52Z

modules/material-management/src/cryptographic_material.ts

+ * The functional data key (unencrypted or CryptoKey) is the most sensitive data and needs to
+ * be protected.  The longer this data persists in memory the
+ * greater the opportunity to be invalidated.  Because
+ * a Caching CMM exists is it important to insure that the


"a Caching CMM exists it is important to ensure that the"

mattsb42-aws · 2019-07-18T23:31:58Z

NOTE: encrypt in encrypt-node now expects encryptionContext instead of context.
This is ONLY done because the project is still in beta.

Agreed, and I think this was the right call.

Just calling out here, though, that even though we're in beta we will still need to update the version appropriately for a breaking change (so this will be 0.2.0)

modules/material-management-browser/src/browser_cryptographic_materials_manager.ts

mattsb42-aws · 2019-07-19T00:40:47Z

modules/material-management-browser/test/browser_cryptographic_materials_manager.test.ts

-    if (!test.material.signatureKey) throw new Error('I should never see this error')
-    expect(test.context).to.have.haveOwnProperty(ENCODED_SIGNER_KEY).and.to.equal(toBase64(test.material.signatureKey.compressPoint))
-    expect(test.context).to.have.haveOwnProperty('some').and.to.equal('context')
+    const { material } = await cmm.getEncryptionMaterials({ suite, encryptionContext })


I know that makes it a bit difficult if we wanted to add something else in to this response later

For the record, this is exactly what the encryption materials structure is for. ;) That's what we need to modify if we want to return more things.

modules/material-management-node/src/node_cryptographic_materials_manager.ts

modules/material-management/src/cryptographic_material.ts

Co-Authored-By: Matt Bullock <bullocm@amazon.com>

…als_manager.ts Co-Authored-By: Matt Bullock <bullocm@amazon.com>

…materials_manager.ts Co-Authored-By: Matt Bullock <bullocm@amazon.com>

seebees · 2019-07-23T00:01:50Z

@mattsb42-aws

Just calling out here, though, that even though we're in beta we will still need to update the version appropriately for a breaking change (so this will be 0.2.0)

To be clear, I'm using: https://www.conventionalcommits.org/en/v1.0.0-beta.4/ so if it was called out as breaking in the commit message, the MAJOR version would be bumped automatically

ragona

Looks good to me. A couple nitpicks, one question, no blockers.

ragona · 2019-07-23T18:28:57Z

modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts

@@ -46,7 +46,7 @@ describe('KmsKeyring: _onDecrypt',
      const clientProvider: any = () => {
        return { decrypt }
        function decrypt ({ CiphertextBlob, EncryptionContext, GrantTokens }: any) {
-          expect(EncryptionContext === context).to.equal(true)
+          expect(EncryptionContext).to.deep.equal(context)


Nitpick: Since you just ran around doing s/context/encryptionContext/ everywhere else, you might consider doing that here and in the other tests where it's still context. I don't think I normally would have even noticed but after reviewing that change in 26 files it stuck out. :P

This is valid. The examples are another case of this.... Sigh. I agree, but I think I'll come back to this later. Since this is large enough it will be easier to change words after a stable position.

#160

+1, definitely not worth holding this up.

ragona · 2019-07-23T18:42:38Z

modules/material-management/test/keyring.test.ts

@@ -46,7 +45,7 @@ describe('Keyring', () => {
  it('onEncrypt calls _onEncrypt', async () => {
    const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
    const { keyLengthBytes } = suite
-    const m = new NodeEncryptionMaterial(suite)
+    const m = new NodeEncryptionMaterial(suite, {})


Nitpick: Would prefer a more descriptive name here.

What are you looking for? Defining the variable as opposed to just passing an object lateral?

Oh, sorry, I meant m not {}. Not a big deal.

ragona · 2019-07-23T18:44:20Z

modules/material-management/test/multi_keyring.test.ts


    const generator = keyRingFactory({
-      async onDecrypt (material: NodeDecryptionMaterial /*, encryptedDataKeys: EncryptedDataKey[], context?: EncryptionContext */) {
+      async onDecrypt (material: NodeDecryptionMaterial /*, encryptedDataKeys: EncryptedDataKey[] */) {


Not really feedback, just curious about this pattern with the commented out parameters; I've seen it in a couple of places. What's the story there?

For me, this is similar to named parameters. Ideally the both the function declaration and the call site should be descriptive of intention.

In this case, the function is "cut off" because I'm only using some of the arguments. My Typescript config is relatively strict and wants every defined variable to be used. So I have to remove the arguments because they are not being used. However when copying this code around or trying to understand what/how it is used, it is useful to see the full definition.

Makes sense!

resolves aws#232 Typescript 3.7 does a better job identifying declaration conflicts and so identifies the conflite between the type and the class import of the same name. e.g. `WebCryptoEncryptionMaterial` is defined as a type and imported. When aws#148 was the referance to the return type can go directly to the needed Material (WebCrypto or Node).

resolves aws#232 Typescript 3.7 does a better job identifying declaration conflicts and so identifies the conflict between the type and the class import of the same name. e.g. `WebCryptoEncryptionMaterial` is defined as a type and imported. When aws#148 was the reference to the return type can go directly to the needed Material (WebCrypto or Node).

resolves aws#232 Typescript 3.7 does a better job identifying declaration conflicts and so identifies the conflict between the type and the class import of the same name. e.g. `WebCryptoEncryptionMaterial` is defined as a type and imported. When aws#148 was written the reference to the return type should have gone directly to the needed Material (WebCrypto or Node).

resolves #232 Typescript 3.7 does a better job identifying declaration conflicts and so identifies the conflict between the type and the class import of the same name. e.g. `WebCryptoEncryptionMaterial` is defined as a type and imported. When #148 was written the reference to the return type should have gone directly to the needed Material (WebCrypto or Node).

seebees requested a review from a team July 15, 2019 17:07

karlw00t reviewed Jul 16, 2019

View reviewed changes

modules/cache-material/test/clone_cryptographic_material.test.ts Show resolved Hide resolved

Merge branch 'master' into encryption_context_on_material

572745e

lavaleri reviewed Jul 16, 2019

View reviewed changes

mattsb42-aws requested changes Jul 19, 2019

View reviewed changes

seebees and others added 8 commits July 22, 2019 15:42

Merge branch 'master' into encryption_context_on_material

56a0a3c

Update modules/material-management/src/cryptographic_material.ts

dd21128

Co-Authored-By: Matt Bullock <bullocm@amazon.com>

Update modules/material-management-node/src/node_cryptographic_materi…

a9b6259

…als_manager.ts Co-Authored-By: Matt Bullock <bullocm@amazon.com>

Update modules/material-management-browser/src/browser_cryptographic_…

2d3d294

…materials_manager.ts Co-Authored-By: Matt Bullock <bullocm@amazon.com>

change interface to return material,not nested material

b4cca21

test pass

3468cb5

no .context

9d3283e

even better name

d8220e3

ragona previously approved these changes Jul 23, 2019

View reviewed changes

Merge branch 'master' into encryption_context_on_material

681963f

lavaleri previously approved these changes Jul 24, 2019

View reviewed changes

insure is not a word

1acb20b

seebees dismissed stale reviews from lavaleri and ragona via 1acb20b July 24, 2019 18:25

seebees added 2 commits July 24, 2019 11:27

more

47afaf2

Merge branch 'master' into encryption_context_on_material

ee4fea3

lavaleri approved these changes Jul 24, 2019

View reviewed changes

Merge branch 'master' into encryption_context_on_material

a4d0c57

mattsb42-aws approved these changes Jul 26, 2019

View reviewed changes

seebees merged commit 5a7e9ca into aws:master Jul 26, 2019

seebees deleted the encryption_context_on_material branch July 26, 2019 21:58

seebees mentioned this pull request Nov 6, 2019

fix: Import declaration conflicts with local declaration #233

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Encryption Context changes #148

fix: Encryption Context changes #148

seebees commented Jul 15, 2019

lavaleri Jul 16, 2019

mattsb42-aws Jul 19, 2019

lavaleri Jul 16, 2019

seebees Jul 24, 2019

mattsb42-aws commented Jul 18, 2019

mattsb42-aws Jul 19, 2019

seebees commented Jul 23, 2019

ragona left a comment

ragona Jul 23, 2019

seebees Jul 23, 2019

ragona Jul 24, 2019

ragona Jul 23, 2019

seebees Jul 23, 2019

ragona Jul 24, 2019

ragona Jul 23, 2019

seebees Jul 23, 2019

ragona Jul 24, 2019

fix: Encryption Context changes #148

fix: Encryption Context changes #148

Conversation

seebees commented Jul 15, 2019

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

mattsb42-aws commented Jul 18, 2019

Choose a reason for hiding this comment

seebees commented Jul 23, 2019

ragona 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

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