Update documentation and warnings related to SaveBehavior. #44

SalusaSecondus · 2018-08-03T18:35:56Z

This is in support of issue #42

This adds basic documentation to AttributeEncryptor and related example code highlighting the importance of using SaveBehavior.CLOBBER.

All existing tests pass. Some of the existing tests did not properly set this configuration and resulted in proper warnings during the test run:

SEVERE: Use of AttributeEncryptor without SaveBehavior.CLOBBER is an error and may result in data-corruption. This occured while trying to save class com.amazonaws.services.dynamodbv2.testing.types.Mixed

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

SalusaSecondus · 2018-08-03T18:54:51Z

src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/AttributeEncryptor.java

-import java.util.HashMap;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;


Bad IntelliJ setting injected an import-*. I'll fix this.

lizroth

Found a few things we might want to polish up. Let me know your thoughts.

lizroth · 2018-08-03T19:05:27Z

README.md

@@ -3,6 +3,7 @@
 The **[Amazon DynamoDB][ddb] Client-side Encryption in Java** supports encryption and signing of your data when stored in Amazon DynamoDB.

 A typical use of this library is when you are using [DynamoDBMapper][ddbmapper], where transparent protection of all objects serialized through the mapper can be enabled via configuring an [AttributeEncryptor][attrencryptor].
+**Please note that it is critically important that you use `SaveBehavior.CLOBBER` when using AttributeEncryptor.**


Can we compact the wording of the warning for grokkability, and add an expanded explanation?

Perhaps:

Important: use SaveBehavior.CLOBBER with AttributeEncryptor. This is because ...

Should https://github.com/aws/aws-dynamodb-encryption-java/blame/ded8364a9baee731f6f83ddddca8cec5bc614f9c/README.md#L77 be DynamoDBMapperConfig.CLOBBER as well? It looks to my quick read like it is affected, since it's using the AttributeEncryptor.

lizroth · 2018-08-03T19:06:31Z

src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/AttributeEncryptor.java


 /**
 * Encrypts all non-key fields prior to storing them in DynamoDB.
+ * <em>It is critically important that this is only used with @{link SaveBehavior#CLOBBER}. Use of


Brevity for grokability?:

<em>This must be used with @{link SaveBehavior#CLOBBER}.</em> Use of any other...

Also, I think June would suggest using "can" rather than "may" for clarity, since it is a possibility, not a permission.

lizroth · 2018-08-03T19:08:42Z

src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/AttributeEncryptor.java

+        }
+
+        if (parameters.isPartialUpdate()) {
+            LOG.error("Use of AttributeEncryptor without SaveBehavior.CLOBBER is an error and may result in data-corruption. " +


s/may/can/?

lizroth · 2018-08-03T19:11:29Z

src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/AttributeEncryptor.java

@@ -58,9 +64,20 @@ public DynamoDBEncryptor getEncryptor() {
    public Map<String, AttributeValue> transform(final Parameters<?> parameters) {
        // one map of attributeFlags per model class
        final ModelClassMetadata metadata = getModelClassMetadata(parameters);
+
+        final Map<String, AttributeValue> attributeValues = parameters.getAttributeValues();


Could you please add a short comment explaining the case being handled here / why this is set to behave this way, for clarity/maintainers?

lizroth · 2018-08-03T19:16:36Z

src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/AttributeEncryptor.java

@@ -58,9 +64,20 @@ public DynamoDBEncryptor getEncryptor() {
    public Map<String, AttributeValue> transform(final Parameters<?> parameters) {
        // one map of attributeFlags per model class
        final ModelClassMetadata metadata = getModelClassMetadata(parameters);
+
+        final Map<String, AttributeValue> attributeValues = parameters.getAttributeValues();
+        if (metadata.doNotTouch) {


This looks like a behavior change, unless I am missing something. Can we cover it?

It isn't a behavior change. In a "DoNotTouch" case, we make no changes whatsoever to the underlying data. I'm just short-circuiting that logic.

lizroth · 2018-08-03T19:21:54Z

examples/com/amazonaws/examples/AwsKmsEncryptedObject.java

@@ -52,6 +52,7 @@ public static void encryptRecord(final String cmkArn, final String region) {
    // Encryptor creation
    final DynamoDBEncryptor encryptor = DynamoDBEncryptor.getInstance(cmp);
    // Mapper Creation
+    // Please note the use of SaveBehavior.CLOBBER. Omitting this may result in data-corruption.


s/may/can/?

…yptor

bdonlan · 2018-08-03T20:40:21Z

README.md

@@ -4,6 +4,8 @@ The **[Amazon DynamoDB][ddb] Client-side Encryption in Java** supports encryptio

 A typical use of this library is when you are using [DynamoDBMapper][ddbmapper], where transparent protection of all objects serialized through the mapper can be enabled via configuring an [AttributeEncryptor][attrencryptor].

+**Important: Use `SaveBehavior.CLOBBER` with `AttributeEncryptor`. If you do not do so you risk corrupting your signatures and encrypted data.**


Explain why this is the case?

Important: Use SaveBehavior.CLOBBER with AttributeEncryptor. If you do not do so you risk corrupting your signatures and encrypted data. When CLOBBER is not specified, fields that are present in the record may not be passed down to the encryptor, which results in fields being left out of the record signature. This in turn can result in records failing to decrypt.

+1 - I want to learn more about why this is the desired behavior

Using CLOBBER basically disables the Optimistic Locking which is a super useful feature provided by DynamoDB. This says we shouldn't use AttributeEncryptor and Optimistic Locking together.

We support partial encryption and signing today, can those fields be updated safely without CLOBBER save?

We will re-visit how optimistic logging works with encrypted data. It definitely will not work without a version field. This change is still important due to the need to warn people about the risk of data-corruption.

yubangxi · 2018-08-03T20:44:32Z

src/main/java/com/amazonaws/services/dynamodbv2/datamodeling/AttributeEncryptor.java

+        // unmodified fields. Thus, upon untransform, the signature verification will fail as it won't cover all
+        // expected fields.
+        if (parameters.isPartialUpdate()) {
+            LOG.error("Use of AttributeEncryptor without SaveBehavior.CLOBBER is an error and can result in data-corruption. " +


For anyone who is not using CLOBBER today, there will be tons of errors in the log and the operation is not actually failing.

That is correct because they are in a dangerous error case and are at risk for data-corruption. Minor software changes outside of their (or our) control could result in this going from a "working due to luck" state to a "broken" state. We consider it sufficiently important to notify people of this dangerous configuration that an ERROR level log is appropriate.

The next minor (non-patch) version will throw an exception in this case. Please see #32

Restore logic to set UA suffixes

ISSUE #42. Update documentation and warnings related to SaveBehavior.

b988c8e

SalusaSecondus commented Aug 3, 2018

View reviewed changes

Remove import-* in AttributeEncryptor.java

b5f7fe9

lizroth suggested changes Aug 3, 2018

View reviewed changes

SalusaSecondus added 3 commits August 3, 2018 12:35

Improve wording around need of SaveBehavior.CLOBBER

8a00b2a

Typo fix in README.md

c1f74d0

Improve explanation of issues when not using CLOBBER in AttributeEncr…

cd1ebb4

…yptor

bdonlan reviewed Aug 3, 2018

View reviewed changes

yubangxi reviewed Aug 3, 2018

View reviewed changes

Improve explanation of SaveBehavior.CLOBBER in README.md

7530a1e

bdonlan approved these changes Aug 3, 2018

View reviewed changes

lizroth approved these changes Aug 3, 2018

View reviewed changes

lizroth merged commit 4d6b40e into aws:master Aug 3, 2018

seebees pushed a commit that referenced this pull request Dec 12, 2023

Merge pull request #44 from bdonlan/new-ctors-ua

26bd3cb

Restore logic to set UA suffixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update documentation and warnings related to SaveBehavior. #44

Update documentation and warnings related to SaveBehavior. #44

SalusaSecondus commented Aug 3, 2018

SalusaSecondus Aug 3, 2018

lizroth left a comment

lizroth Aug 3, 2018

lizroth Aug 3, 2018

lizroth Aug 3, 2018

lizroth Aug 3, 2018

lizroth Aug 3, 2018

SalusaSecondus Aug 3, 2018

lizroth Aug 3, 2018

lizroth Aug 3, 2018

bdonlan Aug 3, 2018

bdonlan Aug 3, 2018 •

edited

Loading

yubangxi Aug 3, 2018

SalusaSecondus Aug 3, 2018

yubangxi Aug 3, 2018

SalusaSecondus Aug 3, 2018

		@@ -4,6 +4,8 @@ The [Amazon DynamoDB][ddb] Client-side Encryption in Java supports encryptio

		A typical use of this library is when you are using [DynamoDBMapper][ddbmapper], where transparent protection of all objects serialized through the mapper can be enabled via configuring an [AttributeEncryptor][attrencryptor].

		Important: Use `SaveBehavior.CLOBBER` with `AttributeEncryptor`. If you do not do so you risk corrupting your signatures and encrypted data.

Update documentation and warnings related to SaveBehavior. #44

Update documentation and warnings related to SaveBehavior. #44

Conversation

SalusaSecondus commented Aug 3, 2018

Choose a reason for hiding this comment

lizroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdonlan Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdonlan Aug 3, 2018 •

edited

Loading