Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Encryption Context changes #148

Merged
merged 15 commits into from
Jul 26, 2019

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Jul 15, 2019

resolves #54

  1. browser/node encrypt function public api should match
  2. Encryption context should only be optional in the public interface
  3. 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.
@seebees seebees requested a review from a team July 15, 2019 17:07
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 })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@mattsb42-aws
Copy link
Member

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)

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 })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/src/cryptographic_material.ts Outdated Show resolved Hide resolved
@seebees
Copy link
Contributor Author

seebees commented Jul 23, 2019

@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
ragona previously approved these changes Jul 23, 2019
Copy link
Contributor

@ragona ragona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, definitely not worth holding this up.

@@ -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, {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Would prefer a more descriptive name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

lavaleri
lavaleri previously approved these changes Jul 24, 2019
@seebees seebees dismissed stale reviews from lavaleri and ragona via 1acb20b July 24, 2019 18:25
@seebees seebees merged commit 5a7e9ca into aws:master Jul 26, 2019
@seebees seebees deleted the encryption_context_on_material branch July 26, 2019 21:58
seebees added a commit to seebees/aws-encryption-sdk-javascript that referenced this pull request Nov 6, 2019
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).
seebees added a commit to seebees/aws-encryption-sdk-javascript that referenced this pull request Nov 6, 2019
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).
seebees added a commit to seebees/aws-encryption-sdk-javascript that referenced this pull request Nov 6, 2019
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).
seebees added a commit that referenced this pull request Nov 7, 2019
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encryption context should be required everywhere but the initial user entry points
5 participants