Merge pull request #44 from SalusaSecondus/issue-42

Update documentation and warnings related to SaveBehavior.

Author: lizroth

README.md

@@ -4,6 +4,9 @@ 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.**
+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.
+
 For more advanced use cases where tighter control over the encryption and signing process is necessary, the low-level [DynamoDBEncryptor][ddbencryptor] can be used directly.
 
 ## Getting Started
@@ -74,7 +77,7 @@ To enable transparent encryption and signing, simply specify the necessary encry
     SecretKey cek = ...;        // Content encrypting key
     SecretKey macKey =  ...;    // Signing key
     EncryptionMaterialsProvider provider = new SymmetricStaticProvider(cek, macKey);
-    mapper = new DynamoDBMapper(client, DynamoDBMapperConfig.DEFAULT,
+    mapper = new DynamoDBMapper(client, DynamoDBMapperConfig.builder().withSaveBehavior(SaveBehavior.CLOBBER).build(),
                 new AttributeEncryptor(provider));
     Book book = new Book();
     book.setId(123);

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 can result in data-corruption.
     DynamoDBMapperConfig mapperConfig = DynamoDBMapperConfig.builder().withSaveBehavior(SaveBehavior.CLOBBER).build();
     DynamoDBMapper mapper = new DynamoDBMapper(ddb, mapperConfig, new AttributeEncryptor(encryptor));
 

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

@@ -21,6 +21,7 @@
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
+import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMapperConfig.SaveBehavior;
 import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMappingsRegistry.Mapping;
 import com.amazonaws.services.dynamodbv2.datamodeling.DynamoDBMappingsRegistry.Mappings;
 import com.amazonaws.services.dynamodbv2.datamodeling.encryption.DoNotEncrypt;
@@ -32,13 +33,18 @@
 import com.amazonaws.services.dynamodbv2.datamodeling.encryption.TableAadOverride;
 import com.amazonaws.services.dynamodbv2.datamodeling.encryption.providers.EncryptionMaterialsProvider;
 import com.amazonaws.services.dynamodbv2.model.AttributeValue;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 /**
  * Encrypts all non-key fields prior to storing them in DynamoDB.
+ * <em>This must be used with @{link SaveBehavior#CLOBBER}. Use of
+ * any other @{code SaveBehavior} can result in data-corruption.</em>
  * 
  * @author Greg Rubin 
  */
 public class AttributeEncryptor implements AttributeTransformer {
+    private static final Log LOG = LogFactory.getLog(AttributeEncryptor.class);
     private final DynamoDBEncryptor encryptor;
     private final Map<Class<?>, ModelClassMetadata> metadataCache = new ConcurrentHashMap<>();
 
@@ -58,9 +64,26 @@ 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 this class is marked as "DoNotTouch" then we know our encryptor will not change it at all
+        // so we may as well fast-return and do nothing. This also avoids emitting errors when they would not apply.
+        if (metadata.doNotTouch) {
+            return attributeValues;
+        }
+
+        // When AttributeEncryptor is used without SaveBehavior.CLOBBER, it is trying to transform only a subset
+        // of the actual fields stored in DynamoDB. This means that the generated signature will not cover any
+        // 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. " +
+                    "This occured while trying to save " + parameters.getModelClass());
+        }
+
         try {
             return encryptor.encryptRecord(
-                    parameters.getAttributeValues(),
+                    attributeValues,
                     metadata.getEncryptionFlags(),
                     paramsToContext(parameters));
         } catch (Exception ex) {

src/test/java/com/amazonaws/services/dynamodbv2/datamodeling/TransformerHolisticTests.java

@@ -115,6 +115,9 @@ public class TransformerHolisticTests {
     private static final EncryptionMaterialsProvider symWrappedProv;
 
     private AmazonDynamoDB client;
+    // AttributeEncryptor *must* be used with SaveBehavior.CLOBBER to avoid the risk of data corruption.
+    private static final DynamoDBMapperConfig CLOBBER_CONFIG =
+            DynamoDBMapperConfig.builder().withSaveBehavior(SaveBehavior.CLOBBER).build();
     private static final BaseClass ENCRYPTED_TEST_VALUE = new BaseClass();
     private static final Mixed MIXED_TEST_VALUE = new Mixed();
     private static final SignOnly SIGNED_TEST_VALUE = new SignOnly();
@@ -292,7 +295,7 @@ public void setUp() {
 
     @Test
     public void simpleSaveLoad() {
-        DynamoDBMapper mapper = new DynamoDBMapper(client, DynamoDBMapperConfig.DEFAULT, new AttributeEncryptor(symProv));
+        DynamoDBMapper mapper = new DynamoDBMapper(client, CLOBBER_CONFIG, new AttributeEncryptor(symProv));
         Mixed obj = new Mixed();
         obj.setHashKey(0);
         obj.setRangeKey(15);
@@ -322,7 +325,7 @@ public void simpleSaveLoad() {
 
     @Test
     public void leadingAndTrailingZeros() {
-        DynamoDBMapper mapper = new DynamoDBMapper(client, DynamoDBMapperConfig.DEFAULT, new AttributeEncryptor(symProv));
+        DynamoDBMapper mapper = new DynamoDBMapper(client, CLOBBER_CONFIG, new AttributeEncryptor(symProv));
         Mixed obj = new Mixed();
         obj.setHashKey(0);
         obj.setRangeKey(15);
@@ -364,7 +367,7 @@ public void leadingAndTrailingZeros() {
 
     @Test
     public void simpleSaveLoadAsym() {
-        DynamoDBMapper mapper = new DynamoDBMapper(client, DynamoDBMapperConfig.DEFAULT, new AttributeEncryptor(asymProv));
+        DynamoDBMapper mapper = new DynamoDBMapper(client, CLOBBER_CONFIG, new AttributeEncryptor(asymProv));
 
         BaseClass obj = new BaseClass();
         obj.setHashKey(0);
@@ -394,7 +397,7 @@ public void simpleSaveLoadAsym() {
 
     @Test
     public void simpleSaveLoadHashOnly() {
-        DynamoDBMapper mapper = new DynamoDBMapper(client, DynamoDBMapperConfig.DEFAULT, new AttributeEncryptor(
+        DynamoDBMapper mapper = new DynamoDBMapper(client, CLOBBER_CONFIG, new AttributeEncryptor(
                 symProv));
 
         HashKeyOnly obj = new HashKeyOnly("");
@@ -411,7 +414,7 @@ public void simpleSaveLoadHashOnly() {
 
     @Test
     public void simpleSaveLoadKeysOnly() {
-        DynamoDBMapper mapper = new DynamoDBMapper(client, DynamoDBMapperConfig.DEFAULT, new AttributeEncryptor(
+        DynamoDBMapper mapper = new DynamoDBMapper(client, CLOBBER_CONFIG, new AttributeEncryptor(
                 asymProv));
 
         KeysOnly obj = new KeysOnly();

src/test/java/com/amazonaws/services/dynamodbv2/testing/FakeParameters.java

@@ -25,14 +25,20 @@
 import com.amazonaws.services.dynamodbv2.model.AttributeValue;
 
 public class FakeParameters<T> {
+    public static <T> AttributeTransformer.Parameters<T> getInstance(Class<T> clazz,
+                                                                     Map<String, AttributeValue> attribs, DynamoDBMapperConfig config, String tableName,
+                                                                     String hashKeyName, String rangeKeyName) {
+        return getInstance(clazz, attribs, config, tableName, hashKeyName, rangeKeyName, false);
+    }
+
     public static <T> AttributeTransformer.Parameters<T> getInstance(Class<T> clazz,
             Map<String, AttributeValue> attribs, DynamoDBMapperConfig config, String tableName,
-            String hashKeyName, String rangeKeyName) {
+            String hashKeyName, String rangeKeyName, boolean isPartialUpdate) {
 
         // We use this relatively insane proxy setup so that modifications to the Parameters
         // interface doesn't break our tests (unless it actually impacts our code).
         FakeParameters<T> fakeParams = new FakeParameters<T>(clazz, attribs, config, tableName,
-                hashKeyName, rangeKeyName);
+                hashKeyName, rangeKeyName, isPartialUpdate);
         @SuppressWarnings("unchecked")
         AttributeTransformer.Parameters<T> proxyObject = (AttributeTransformer.Parameters<T>) Proxy
                 .newProxyInstance(AttributeTransformer.class.getClassLoader(),
@@ -65,16 +71,19 @@ public Object invoke(Object obj, Method method, Object[] args) throws Throwable
     private final String tableName;
     private final String hashKeyName;
     private final String rangeKeyName;
+    private final boolean isPartialUpdate;
 
     private FakeParameters(Class<T> clazz, Map<String, AttributeValue> attribs,
-            DynamoDBMapperConfig config, String tableName, String hashKeyName, String rangeKeyName) {
+            DynamoDBMapperConfig config, String tableName, String hashKeyName, String rangeKeyName,
+            boolean isPartialUpdate) {
         super();
         this.clazz = clazz;
         this.attrs = Collections.unmodifiableMap(attribs);
         this.config = config;
         this.tableName = tableName;
         this.hashKeyName = hashKeyName;
         this.rangeKeyName = rangeKeyName;
+        this.isPartialUpdate = isPartialUpdate;
     }
 
     public Map<String, AttributeValue> getAttributeValues() {
@@ -100,4 +109,8 @@ public String getHashKeyName() {
     public String getRangeKeyName() {
         return rangeKeyName;
     }
+
+    public boolean isPartialUpdate() {
+        return isPartialUpdate;
+    }
 }