-
Notifications
You must be signed in to change notification settings - Fork 63
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
* and does not leak information about contents. | ||
*/ | ||
if (a.byteLength !== b.byteLength) return false | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes
yes when you create an object with |
||
}) | ||
|
||
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.') | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
}) | ||
|
||
|
@@ -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.', () => { | ||
|
@@ -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) | ||
|
@@ -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.', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = {} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...