fix: Conditions for materials-management #185

seebees · 2019-08-04T23:39:41Z

Every condition needs a test.
This also uncovered some missing conditions and tests.

If a multi keyring has errors AND fails to decrypt the EDK it should throw and error.
Lower the test_conditions threshold

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

Check any applicable:

Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

Every condition needs a test. This also uncovered some missing conditions and tests. * If a multi keyring has errors AND fails to decrypt the EDK it should throw and error. * Lower the test_conditions threshold

lavaleri · 2019-08-06T19:01:30Z

modules/material-management/src/cryptographic_material.ts

@@ -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.


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

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...

lavaleri · 2019-08-06T19:02:08Z

modules/material-management/src/cryptographic_material.ts

@@ -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.


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

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.

lavaleri · 2019-08-06T19:03:12Z

modules/material-management/src/keyring.ts

@@ -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.


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

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...

lavaleri · 2019-08-06T19:05:28Z

modules/material-management/src/multi_keyring.ts

+     * However, if I was unable to decrypt a key AND I have errors,
+     * these errors should bubble up.
+     */
+    needs(material.hasValidKey() || !keyringError, 'Keyring errors and no keyring was successfully able to decrypt the encrypted data key.')


naive JS question: How does JS handle the bubbling up of errors? If this needs condition is not met due to an error caught on L122, how does the error (and possibly others) propagate up, and how is that error handled?

The errors from L122 are lost and not wrapped. That is not a bad idea to add, but it will require some other re-work

lavaleri · 2019-08-06T19:24:59Z

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

@@ -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')


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?

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. :)

lavaleri · 2019-08-06T19:35:56Z

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

    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.


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?

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.

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.

lavaleri · 2019-08-06T19:47:55Z

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

@@ -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.', () => {


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.

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.

gotcha 👍

lavaleri · 2019-08-06T19:54:49Z

package.json

@@ -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 2"


What does this option do?

I am trying to get rid of it. I just lets the test not fail if there are "exceptions" I want to remove it so that no one even sees it and thinks, "Hey, I can get aways with not writing a test by setting this..." But as I get to that point, I want to have it running...

lavaleri · 2019-08-06T19:55:17Z

modules/raw-rsa-keyring-browser/test/get_import_options.test.ts

@@ -27,7 +27,7 @@ chai.use(chaiAsPromised)
 const { expect } = chai

 describe('flattenMixedCryptoKey', () => {
-  it('Check for early return (Postcondition): empty inputs should return an empty array.', () => {
+  it('Check for early return (Postcondition): nothing is an empty array.', () => {


I think this wording changed recently. Did you intend to change it back?

Hum, the test must match the condition. You may be right....

lavaleri · 2019-08-06T19:56:27Z

modules/material-management/src/multi_keyring.ts

      }
    }
+
+    /* Postcondition: Need either a valid data key or no errors.


This logic is in line with what is currently in the spec PR. However, I either want someone familiar with the C multikeyring implementation to either give this logic the OK, or give the current wording in the spec the OK, before I approve this. I will tag someone in the spec CR on this issue.

lavaleri · 2019-08-06T22:41:20Z

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

    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.


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.

fix: Conditions for materials-management

39e0c27

Every condition needs a test. This also uncovered some missing conditions and tests. * If a multi keyring has errors AND fails to decrypt the EDK it should throw and error. * Lower the test_conditions threshold

seebees requested a review from a team August 4, 2019 23:39

lavaleri suggested changes Aug 6, 2019

View reviewed changes

seebees and others added 4 commits August 6, 2019 15:26

Merge branch 'master' into conditions-material-management

a37dbb2

remove, they all should be done now

958aa66

Remove and move to seperate PR

310dc45

better words

3a930a6

lavaleri approved these changes Aug 6, 2019

View reviewed changes

seebees merged commit 7f7228b into aws:master Aug 6, 2019

seebees deleted the conditions-material-management branch August 6, 2019 23:17

fix: Conditions for materials-management #185

fix: Conditions for materials-management #185

Uh oh!

Conversation

seebees commented Aug 4, 2019

Check any applicable:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!