Fixed second round of PR comments

Author: Anirav Kareddy

src/main/java/software/amazon/encryption/s3/internal/ContentMetadata.java

@@ -15,6 +15,12 @@ public class ContentMetadata {
 
     private final EncryptedDataKey _encryptedDataKey;
     private final String _encryptedDataKeyAlgorithm;
+
+    /**
+     * This field stores either encryption context or material description.
+     * We use a single field to store both in order to maintain backwards
+     * compatibility with V2, which treated both as the same.
+     */
     private final Map<String, String> _encryptionContextOrMatDesc;
 
     private final byte[] _contentIv;

src/main/java/software/amazon/encryption/s3/materials/AesKeyring.java

@@ -90,7 +90,7 @@ public boolean isLegacy() {
 
         @Override
         public EncryptionMaterials modifyMaterials(EncryptionMaterials materials) {
-            return modifyMaterialHelper(materials);
+            return modifyMaterialsForRawKeyring(materials);
         }
 
         @Override

src/main/java/software/amazon/encryption/s3/materials/EncryptionMaterials.java

@@ -4,6 +4,7 @@
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import software.amazon.awssdk.services.s3.model.S3Request;
+import software.amazon.encryption.s3.S3EncryptionClientException;
 import software.amazon.encryption.s3.algorithms.AlgorithmSuite;
 import software.amazon.encryption.s3.internal.CipherMode;
 import software.amazon.encryption.s3.internal.CipherProvider;
@@ -187,6 +188,9 @@ public Builder plaintextLength(long plaintextLength) {
         }
 
         public EncryptionMaterials build() {
+            if (!_materialsDescription.isEmpty() && !_encryptionContext.isEmpty()) {
+                throw new S3EncryptionClientException("MaterialsDescription and EncryptionContext cannot both be set!");
+            }
             return new EncryptionMaterials(this);
         }
     }

src/main/java/software/amazon/encryption/s3/materials/MaterialsDescription.java

@@ -54,7 +54,7 @@ public String get(Object key) {
 
   @Override
   public String put(String key, String value) {
-    return materialsDescription.put(key, value);
+    throw new UnsupportedOperationException("This map is immutable");
   }
 
   @Override
@@ -64,7 +64,7 @@ public String remove(Object key) {
 
   @Override
   public void putAll(Map<? extends String, ? extends String> m) {
-      materialsDescription.putAll(m);
+      throw new UnsupportedOperationException("This map is immutable");
   }
 
   @Override
@@ -101,9 +101,7 @@ public Builder putAll(Map<String, String> description) {
           if (description == null) {
             throw new IllegalArgumentException("Description must not be null");
           }
-          for (Map.Entry<String, String> entry : description.entrySet()) {
-            put(entry.getKey(), entry.getValue());
-          }
+          materialsDescription.putAll(description);
           return this;
         }
         public MaterialsDescription build() {

src/main/java/software/amazon/encryption/s3/materials/RawKeyring.java

@@ -10,42 +10,46 @@
  */
 public abstract class RawKeyring extends S3Keyring {
   protected final MaterialsDescription _materialsDescription;
-  protected final boolean _reEncryptInstructionFile;
 
   protected RawKeyring(Builder<?, ?> builder) {
     super(builder);
     _materialsDescription = builder._materialsDescription;
-    _reEncryptInstructionFile = builder._reEncryptInstructionFile;
   }
-  public MaterialsDescription getMaterialsDescription() {
-    return _materialsDescription;
-  }
-  public boolean getReEncryptInstructionFile() {
-    return _reEncryptInstructionFile;
-  }
-  public EncryptionMaterials modifyMaterialHelper(EncryptionMaterials materials) {
+
+  public EncryptionMaterials modifyMaterialsForRawKeyring(EncryptionMaterials materials) {
     warnIfEncryptionContextIsPresent(materials);
     if (_materialsDescription != null && !_materialsDescription.isEmpty()) {
       materials = materials.toBuilder()
         .materialsDescription(_materialsDescription)
         .build();
-      return materials;
     }
-
     return materials;
   }
+
+  /**
+   * Checks if an encryption context is present in the EncryptionMaterials and issues a warning
+   * if an encryption context is found.
+   * <p>
+   * Encryption context is not recommended for use with
+   * non-KMS keyrings as it does not provide additional security benefits and is not stored.
+   *
+   * @param materials EncryptionMaterials
+   */
+
   public void warnIfEncryptionContextIsPresent(EncryptionMaterials materials) {
     materials.s3Request().overrideConfiguration()
       .flatMap(overrideConfiguration ->
         overrideConfiguration.executionAttributes()
           .getOptionalAttribute(S3EncryptionClient.ENCRYPTION_CONTEXT))
-      .ifPresent(ctx -> LogFactory.getLog(getClass()).warn("Usage of Encryption Context provides no security benefit in " + getClass().getSimpleName()));
+      .ifPresent(ctx -> LogFactory.getLog(getClass()).warn("Usage of Encryption Context provides no " +
+        "security benefit in " + getClass().getSimpleName() + ".Additionally, this Encryption Context WILL NOT be " +
+        "stored in the material description. Provide a MaterialDescription in the Keyring's builder instead."));
   }
+
   public static abstract class Builder<KeyringT extends RawKeyring, BuilderT extends Builder<KeyringT, BuilderT>>
       extends S3Keyring.Builder<KeyringT, BuilderT> {
 
     protected MaterialsDescription _materialsDescription;
-    protected boolean _reEncryptInstructionFile = false;
 
     protected Builder() {
       super();
@@ -55,10 +59,5 @@ public BuilderT materialsDescription(MaterialsDescription materialsDescription)
       _materialsDescription = materialsDescription;
       return builder();
     }
-
-    public BuilderT reEncryptInstructionFile(boolean reEncryptInstructionFile) {
-      _reEncryptInstructionFile = reEncryptInstructionFile;
-      return builder();
-    }
   }
 }

src/main/java/software/amazon/encryption/s3/materials/RsaKeyring.java

@@ -95,7 +95,7 @@ public boolean isLegacy() {
 
         @Override
         public EncryptionMaterials modifyMaterials(EncryptionMaterials materials) {
-            return modifyMaterialHelper(materials);
+            return modifyMaterialsForRawKeyring(materials);
         }
 
         @Override

src/main/java/software/amazon/encryption/s3/materials/S3Keyring.java

@@ -23,13 +23,11 @@ abstract public class S3Keyring implements Keyring {
     protected final DataKeyGenerator _dataKeyGenerator;
     private final boolean _enableLegacyWrappingAlgorithms;
     private final SecureRandom _secureRandom;
-    protected final MaterialsDescription _materialsDescription;
 
     protected S3Keyring(Builder<?, ?> builder) {
         _enableLegacyWrappingAlgorithms = builder._enableLegacyWrappingAlgorithms;
         _secureRandom = builder._secureRandom;
         _dataKeyGenerator = builder._dataKeyGenerator;
-        _materialsDescription = builder._materialsDescription;
     }
 
     /**
@@ -130,8 +128,6 @@ abstract public static class Builder<KeyringT extends S3Keyring, BuilderT extend
         private boolean _enableLegacyWrappingAlgorithms = false;
         private SecureRandom _secureRandom;
         private DataKeyGenerator _dataKeyGenerator = new DefaultDataKeyGenerator();
-        protected MaterialsDescription _materialsDescription;
-
 
         protected Builder() {}
 
@@ -162,11 +158,6 @@ public BuilderT dataKeyGenerator(final DataKeyGenerator dataKeyGenerator) {
             _dataKeyGenerator = dataKeyGenerator;
             return builder();
         }
-        public BuilderT materialsDescription(final MaterialsDescription materialsDescription) {
-            _materialsDescription = materialsDescription;
-            return builder();
-        }
-
         abstract public KeyringT build();
     }
 }