NPE if reading record without a amzn-ddb-map-desc

[lavaleri]

This line throws a null pointer exception if you attempt to decrypt a record where the amzn-ddb-map-desc attribute has been manually removed: https://github.com/aws/aws-dynamodb-encryption-java/blob/master/sdk1/src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/encryption/providers/DirectKmsMaterialProvider.java#L126

We should check this earlier and throw a more useful exception.

[bhuvangu]

It should not throw an error and should read the document correctly.
Please consider below scenarios

1. Developer uses DynamoDB to save personDetails table with attributes as Id, Name, Age.
2. Developer Had no requirement of encryption so uses the ddbMapper as

dynamoDBMapper = new DynamoDBMapper(db, dynamoDBMapperConfig);

This causes the document to be saved without amzn-ddb-map-desc

NOW evolution happens and developer need to save an extra attribute Bank Account No and this needs to be encrypted.
So, we change the ddbMapper like

dynamoDBMapper = new DynamoDBMapper(db,
                                    dynamoDBMapperConfig,
                                    new AttributeEncryptor(LET_SAY_KMS_Encryptor));

Now new document will be saved with amzn-ddb-map-desc.

BUT Now we will not be able to read the previously saved document.
As we will get NPE as mentioned by @lavaleri

[lavaleri]

Hi @bhuvangu,

Thank you for providing an additional use case where this NPE shows up. I have confirmed this behavior on my end, and I agree it should be considered a bug. According to our documentation, adding a new field in this way should work (this is equivalent to first configuring the data with @DoNotTouch on every field, then adding a new field).

We need to further investigate what the best way to fix this is, however I think it will involve updating the logic here to also recognize if itemAttributes doesn't contain any field with a ENCRYPT or SIGN flag in the corresponding attributesFlag value.

[bhuvangu]

Hi @lavaleri

checking if itemAttributes doesn't contain any field with a ENCRYPT or SIGN.

It will still not help in the case mentioned above, as Bank Account No field will be annotated with ENCRYPT.

I think in order to keep it backward compatible and fix this bug, we can add a param in DynamoDBMapperConfig as DynamoDBMapperConfig.DecryptPolicy and DecryptPolicy can be STRICT, RELAXED.
Then in AttributeEncryptor.untransform method we can encapsulate the policy as a flag in EncryptionContext and pass to DynamoDBEncryptor.decryptRecord and we can check for the flag in decryptRecord method as shown below

if (itemAttributes does not contains [DEFAULT_SIGNATURE_FIELD, DEFAULT_METADATA_FIELD]
    && encryptionContext.allowRelaxedRead) {
      return itemAttributes;
    }

How does it look , will raise pull request if the idea looks convincing.

[lavaleri]

Hi @bhuvangu,

Thanks for your suggestion. It looks like that approach would work, however I would prefer to fix the underlying issue without adding new modes of operation to our API. To clarify what I meant for the fix:

On entering decryptRecord we have itemAttributes which contains a map of attribute names to their corresponding AttributeValues, and attributeFlags, which should contain a map of each attribute name to ENCRYPT and/or SIGN. Importantly, for an older item a new field like Bank Account No would NOT appear in itemAttributes. For such an item, even though there is a Bank Account No field in the attributeFlags, we would want pass through behavior, as there is no attribute name that exists in itemAttributes that has a corresponding ENCRYPT/SIGN in attributeFlags.

[bhuvangu]

hi @lavaleri ,
Got it!!! tested it with live dynamoDB and what you said is correct and works.
Created a PR for the same with unit test. Let me know if some improvement is required in the PR

[lavaleri]

As of #152 we have fixed one bug where an NPE pops up due to a missing signature/matdesc field in the case of upgrading from a "DoNotTouch" schema.

The specific NPE thrown at https://github.com/aws/aws-dynamodb-encryption-java/blob/master/sdk1/src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/encryption/providers/DirectKmsMaterialProvider.java#L126 is still an issue in that we should instead throw an IllegalArgumentException instead of a NPE.

In order to not introduce breaking changes for customers who are not using KMS providers, I think the solution for this will be to check specifically for a null value in the KMS provider and throw the appropriate error.