Skip to content

fix: Conditions for materials-management #185

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

Merged
merged 5 commits into from
Aug 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions modules/decrypt-node/test/decipher_stream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,4 @@ describe('getDecipherStream', () => {

await expect(test._onAuthTag(Buffer.from([]), () => {})).to.rejectedWith(Error, 'AuthTag before frame.')
})

it('Precondition: I must have received all content for this frame.', async () => {
/* The fact that I can not figure out how to test this,
* makes me want to remove the condition.
* However, it is very important to make sure that the entire frame has been accumulated.
* I suspect that the fact that frameComplete is a closure,
* means that this is impossible.
* This kind of non-test is also included in the cryptographic materials
* for a similar kind of closure around the udkForVerification.
*/
})
})
7 changes: 4 additions & 3 deletions modules/material-management/src/cryptographic_material.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function portableTimingSafeEqual (a: Uint8Array, b: Uint8Array) {
* Side channel attacks are pernicious and subtle.
*/
eval('') // eslint-disable-line no-eval
/* Check for early return (Postcondition): Size is well-know information
/* Check for early return (Postcondition) UNTESTED: Size is well-know information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this untested? Is there a reason this is not testable, or is adding the test outside the scope of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because constant time function are not verifiable (as I understand it). So this function is not exported so it can not be used elsewhere.

The fact that I return in this place is something that needs to be explained, but I don't even know how I would verify this one way or another...

* and does not leak information about contents.
*/
if (a.byteLength !== b.byteLength) return false
Expand Down Expand Up @@ -338,8 +338,9 @@ export function decorateCryptographicMaterial<T extends CryptographicMaterial<T>
udkForVerification.fill(0)
unencryptedDataKeyZeroed = true

/* Postcondition: Both unencryptedDataKey and udkForVerification must be either set or unset.
/* Postcondition UNTESTED: Both unencryptedDataKey and udkForVerification must be either set or unset.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this untested? Is there a reason this is not testable, or is adding the test outside the scope of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes: It is not clear how this could ever happen, unless someone is manipulating the OS...

The values are private, I have no way to manipulate the values to verify that the function will indeed fail in this case.

* If it is ever the case that only one was unset, then something is wrong in a profound way.
* It is not clear how this could ever happen, unless someone is manipulating the OS...
*/
needs(unsetCount === 0 || unsetCount === 2, 'Either unencryptedDataKey or udkForVerification was not set.')
return material
Expand Down Expand Up @@ -493,7 +494,7 @@ export function decorateWebCryptoMaterial<T extends WebCryptoMaterial<T>> (mater
if (!material.hasUnencryptedDataKey) {
/* Precondition: If the CryptoKey is the only version, the trace information must be set here. */
needs(trace && trace.keyName && trace.keyNamespace, 'Malformed KeyringTrace')
/* Precondition: On set the required KeyringTraceFlag must be set. */
/* Precondition: On setting the CryptoKey the required KeyringTraceFlag must be set. */
needs(trace.flags & setFlags, 'Required KeyringTraceFlag not set')
/* If I a setting a cryptoKey without an unencrypted data key,
* an unencrypted data should never be set.
Expand Down
2 changes: 1 addition & 1 deletion modules/material-management/src/keyring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export abstract class Keyring<S extends SupportedAlgorithmSuites> {
*/
needs(material === _material, 'New EncryptionMaterial instances can not be created.')

/* Postcondition: If this keyring generated data key, it must be the right length.
/* Postcondition UNTESTED: If this keyring generated data key, it must be the right length.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this untested? Is there a reason this is not testable, or is adding the test outside the scope of this PR?

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 covered by the materials themselves. The comment is there to make sure that people know it needs to be verified. But there is no code to be tested...

* See cryptographic_materials.ts This is handled in setUnencryptedDataKey
* this condition is listed here to keep help keep track of important conditions
*/
Expand Down
14 changes: 13 additions & 1 deletion modules/material-management/test/algorithm_suites.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,22 @@ describe('AlgorithmSuiteIdentifier', () => {
describe('AlgorithmSuite', () => {
it('should not allow an instance', () => {
// @ts-ignore Trying to test something that Typescript should deny...
expect(() => new AlgorithmSuite()).to.throw()
expect(() => new AlgorithmSuite()).to.throw('new AlgorithmSuite is not allowed')
Copy link
Contributor

Choose a reason for hiding this comment

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

Another niave JS question, what is going on here: https://github.com/awslabs/aws-encryption-sdk-javascript/blob/master/modules/material-management/src/algorithm_suites.ts#L105

Is this preventing the AlgorithmSuite constructor from being called? Is this because you're trying to ensure that only extensions of AlgorithmSuite are actually constructed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this preventing the AlgorithmSuite constructor from being called?

yes

Is this because you're trying to ensure that only extensions of AlgorithmSuite are actually constructed?

yes

when you create an object with new there is a special property constructor that is just what it says. :)

})

it('prototype should be immutable', () => {
expect(Object.isFrozen(AlgorithmSuite.prototype))
})

it('Precondition: A algorithm suite specification must be passed.', () => {
class Test extends AlgorithmSuite {}

expect(() => new Test(undefined as any)).to.throw('Algorithm specification not set.')
})

it('Precondition: The Algorithm Suite Identifier must exist.', () => {
class Test extends AlgorithmSuite {}

expect(() => new Test({ id: 'does not exist' } as any)).to.throw('No suite by that identifier exists.')
})
})
70 changes: 67 additions & 3 deletions modules/material-management/test/cryptographic_material.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,24 @@ describe('decorateCryptographicMaterial', () => {
expect(() => test.unencryptedDataKeyLength).to.throw()
})

it('Precondition: If the unencryptedDataKey has not been set, it should not be settable later.', () => {
it('Precondition: the unencryptedDataKey must not be Zeroed out.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const test = decorateCryptographicMaterial((<any>{ suite, keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY)
const dataKey = new Uint8Array(suite.keyLengthBytes).fill(1)
const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY }
test.setUnencryptedDataKey(dataKey, trace)
test.zeroUnencryptedDataKey()
expect(() => test.unencryptedDataKeyLength).to.throw('unencryptedDataKey has been zeroed.')
})

it(`Precondition: If the unencryptedDataKey has not been set, it should not be settable later.
Precondition: If the udkForVerification has not been set, it should not be settable later.`, () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const test = decorateCryptographicMaterial((<any>{ suite, keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY)
test.zeroUnencryptedDataKey()
const dataKey = new Uint8Array(suite.keyLengthBytes).fill(1)
const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY }
// It is very hard to test this perfectly. However, this tests the spirit.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the "spirit" that is actually being tested here, and what test is missing for the above preconditions?

This looks to me like were testing the postcondition at https://github.com/awslabs/aws-encryption-sdk-javascript/blob/master/modules/material-management/src/cryptographic_material.ts#L341

I'm not familiar with the precise definition of pre/post condition as used in this codebase, but if we aren't checking something and throwing an error on the precondition does it make sense to call it a precondition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the unencryptedDataKey has not been set, it should not be settable later.

i.e. if you create materials, then test.zeroUnencryptedDataKey(), you should not be able to set an unencrypted data key later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see the behavior now. This test depends on setUnencryptedDataKey maintaining a behavior where it checks whether the unencryptedDataKey/udkForVerification has been set. This makes the test more brittle than I'd like for the precondition as a whole, but at the very least we ensure with this test that the case really we care about ("you can't set after zeroing) is taken care of.

expect(() => test.setUnencryptedDataKey(dataKey, trace)).to.throw()
})

Expand All @@ -116,8 +128,32 @@ describe('decorateCryptographicMaterial', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const test = decorateCryptographicMaterial((<any>{ suite, keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY)
const dataKey = new Uint8Array(suite.keyLengthBytes).fill(1)
test.setUnencryptedDataKey(dataKey, { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY })
expect(() => test.setUnencryptedDataKey(dataKey)).to.throw()
const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY }
test.setUnencryptedDataKey(dataKey, trace)
expect(() => test.setUnencryptedDataKey(dataKey, trace)).to.throw('unencryptedDataKey has already been set')
})

it('Precondition: dataKey should have an ArrayBuffer that *only* stores the key.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const test = decorateCryptographicMaterial((<any>{ suite, keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY)
const dataKey = new Uint8Array(new ArrayBuffer(suite.keyLengthBytes + 10), 5, suite.keyLengthBytes).fill(1)
const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY }
expect(() => test.setUnencryptedDataKey(dataKey, trace)).to.throw('Unencrypted Master Key must be an isolated buffer.')
})

it('Precondition: Trace must be set, and the flag must indicate that the data key was generated.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const test = decorateCryptographicMaterial((<any>{ suite, keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY)
const dataKey = new Uint8Array(suite.keyLengthBytes).fill(1)
expect(() => test.setUnencryptedDataKey(dataKey, {} as any)).to.throw('Malformed KeyringTrace')
})

it('Precondition: On set the required KeyringTraceFlag must be set.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const test = decorateCryptographicMaterial((<any>{ suite, keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY)
const dataKey = new Uint8Array(suite.keyLengthBytes).fill(1)
const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_SIGNED_ENC_CTX }
expect(() => test.setUnencryptedDataKey(dataKey, trace)).to.throw('Required KeyringTraceFlag not set')
})

it('Precondition: The unencryptedDataKey must not have been modified.', () => {
Expand Down Expand Up @@ -172,6 +208,13 @@ describe('decorateEncryptionMaterial', () => {
expect(() => test.addEncryptedDataKey(edk, KeyringTraceFlag.WRAPPING_KEY_ENCRYPTED_DATA_KEY)).to.throw()
})

it('Precondition: flags must indicate that the key was encrypted.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const test: any = decorateEncryptionMaterial((<any>{ suite, keyringTrace: [], hasUnencryptedDataKey: true }))
const edk = new EncryptedDataKey({ providerId: 'p', providerInfo: 'p', encryptedDataKey: new Uint8Array(3) })
expect(() => test.addEncryptedDataKey(edk, KeyringTraceFlag.WRAPPING_KEY_VERIFIED_ENC_CTX)).to.throw('Encrypted data key flag must be set.')
})

it('Precondition: The SignatureKey stored must agree with the algorithm specification.', () => {
const suiteWithSig = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES256_GCM_IV12_TAG16_HKDF_SHA384_ECDSA_P384)
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
Expand Down Expand Up @@ -309,6 +352,27 @@ describe('decorateWebCryptoMaterial', () => {
expect(() => test.setCryptoKey(key2, trace)).to.throw()
})

it('Precondition: If the CryptoKey is the only version, the trace information must be set here.', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is unclear to me what "If the CryptoKey is the only version" means. Looking at the code, this should be a precondition about the keyring trace during setCryptoKey if the udk isn't already set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you only set a CryptoKey. And do not set an unencrypted data key. This happens in browsers when an EDK can be "unwrapped". The API lets you do this unwrapping and put the data key into an un exportable CryptoKey. This means that the unencrypted data key is never available to the javascript.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha 👍

const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16_HKDF_SHA256_ECDSA_P256)
const test: any = decorateWebCryptoMaterial((<any>{ suite, validUsages: ['deriveKey'], keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY)
decorateCryptographicMaterial(test, KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY)

const key: any = { type: 'secret', algorithm: { name: 'HKDF' }, usages: ['deriveKey'], extractable: false }
expect(() => test.setCryptoKey(key, { keyNamespace: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY })).to.throw('Malformed KeyringTrace')
expect(() => test.setCryptoKey(key, { keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY })).to.throw('Malformed KeyringTrace')
expect(() => test.setCryptoKey(key)).to.throw('Malformed KeyringTrace')
})

it('Precondition: On setting the CryptoKey the required KeyringTraceFlag must be set.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16_HKDF_SHA256_ECDSA_P256)
const test: any = decorateWebCryptoMaterial((<any>{ suite, validUsages: ['deriveKey'], keyringTrace: [] }), KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY)
decorateCryptographicMaterial(test, KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY)

const key: any = { type: 'secret', algorithm: { name: 'HKDF' }, usages: ['deriveKey'], extractable: false }
const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_SIGNED_ENC_CTX }
expect(() => test.setCryptoKey(key, trace)).to.throw('Required KeyringTraceFlag not set')
})

it('Precondition: dataKey must be a supported type.', () => {
const test: any = decorateWebCryptoMaterial((<any>{}), KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY)
const key: any = {}
Expand Down
7 changes: 4 additions & 3 deletions modules/material-management/test/multi_keyring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ describe('MultiKeyring: onDecrypt', () => {
expect(test.getUnencryptedDataKey()).to.deep.equal(unencryptedDataKey)
})

it('does not call subsequent Keyrings after receiving DecryptionMaterial', async () => {
it('Check for early return (Postcondition): Do not attempt to decrypt once I have a valid key.', async () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
const unencryptedDataKey = new Uint8Array(suite.keyLengthBytes)
const [edk0, keyringTrace0] = makeEDKandTrace(0)
Expand Down Expand Up @@ -338,13 +338,14 @@ describe('MultiKeyring: onDecrypt', () => {
})

let called = false
const childNotSucceded = keyRingFactory({
const childNotSucceeded = keyRingFactory({
async onDecrypt () {
// Because this keyring does not return a value, it will result in an error
called = true
},
onEncrypt: never
})
const children = [childNotSucceded, child]
const children = [childNotSucceeded, child]

const mkeyring = new MultiKeyringNode({ children })

Expand Down
22 changes: 22 additions & 0 deletions modules/material-management/test/signature_key.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,17 @@ describe('SignatureKey', () => {
const compressPoint = SignatureKey.encodeCompressPoint(publicKey, suite)
expect(compressPoint).to.deep.equal(new Uint8Array(prime256v1CompressedFixture))
})

it('Precondition: Do not create a SignatureKey for an algorithm suite that does not have an EC named curve.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
expect(() => new SignatureKey(new Uint8Array(3), new Uint8Array(3), suite)).to.throw('Unsupported Algorithm')
})

it('Precondition: Do not return a compress point for an algorithm suite that does not have an EC named curve.', () => {
const publicKey = new Uint8Array(prime256v1PublicFixture)
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
expect(() => SignatureKey.encodeCompressPoint(publicKey, suite)).to.throw('Unsupported Algorithm')
})
})

describe('VerificationKey', () => {
Expand All @@ -65,4 +76,15 @@ describe('VerificationKey', () => {
const publicKey = VerificationKey.decodeCompressPoint(compressPoint, suite)
expect(publicKey).to.deep.equal(new Uint8Array(prime256v1PublicFixture))
})

it('Precondition: Do not create a VerificationKey for an algorithm suite that does not have an EC named curve.', () => {
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
expect(() => new VerificationKey(new Uint8Array(3), suite)).to.throw('Unsupported Algorithm')
})

it('Precondition: Do not decode a public key for an algorithm suite that does not have an EC named curve.', () => {
const compressPoint = new Uint8Array(prime256v1CompressedFixture)
const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
expect(() => VerificationKey.decodeCompressPoint(compressPoint, suite)).to.throw('Unsupported Algorithm')
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export function verify (...args: RsaWrappingKeyAlgorithm[]) {
}

export function flattenMixedCryptoKey (key?: CryptoKey|MixedBackendCryptoKey): CryptoKey[] {
/* Check for early return (Postcondition): nothing is an empty array. */
/* Check for early return (Postcondition): empty inputs should return an empty array. */
if (!key) return []
if (isCryptoKey(key)) return [key]
const { nonZeroByteCryptoKey, zeroByteCryptoKey } = key
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"integration-node-encrypt": "npm run build; lerna run integration_node --stream --no-prefix -- -- encrypt -m $npm_package_config_encryptManifestList -k $npm_package_config_encryptKeyManifest -o $npm_package_config_decryptOracle",
"integration-node": "run-s integration-node-*",
"integration": "run-s integration-*",
"test_conditions": "./util/bootstrap_tsconfig 23"
"test_conditions": "./util/bootstrap_tsconfig"
},
"config": {
"localTestVectors": "../../aws-encryption-sdk-test-vectors/vectors/awses-decrypt/python-1.3.8.zip",
Expand Down