test: Add SpotBugs (#35)

* test: Add SpotBugs, minor changes to comply with SpotBugs

Author: lucasmcdonald3

.github/workflows/ci-test.yml

@@ -20,6 +20,8 @@ jobs:
           aws-region: us-west-2
 
       - uses: actions/checkout@v3
+      - name: Run SpotBugs
+        run: mvn -B -ntp com.github.spotbugs:spotbugs-maven-plugin:check
       # run all test cases
       - name: Run S3EC Test Cases
         run: |

pom.xml

@@ -74,6 +74,20 @@
       <optional>true</optional>
     </dependency>
 
+    <dependency>
+      <groupId>net.jcip</groupId>
+      <artifactId>jcip-annotations</artifactId>
+      <version>1.0</version>
+      <optional>true</optional>
+    </dependency>
+    
+    <dependency>
+      <groupId>com.github.spotbugs</groupId>
+      <artifactId>spotbugs-annotations</artifactId>
+      <version>4.7.3</version>
+      <optional>true</optional>
+    </dependency>
+
     <!-- Test Dependencies -->
     <!-- https://mvnrepository.com/artifact/org.mockito/mockito-core -->
     <dependency>

src/main/java/software/amazon/encryption/s3/S3EncryptionClient.java

@@ -1,9 +1,12 @@
 package software.amazon.encryption.s3;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.security.KeyPair;
 import java.util.Map;
 import java.util.function.Consumer;
 import javax.crypto.SecretKey;
+
 import software.amazon.awssdk.awscore.AwsRequestOverrideConfiguration;
 import software.amazon.awssdk.awscore.exception.AwsServiceException;
 import software.amazon.awssdk.core.exception.SdkClientException;
@@ -122,6 +125,11 @@ public static class Builder {
 
         private Builder() {}
 
+        /**
+         * Note that this does NOT create a defensive clone of S3Client. Any modifications made to the wrapped
+         * S3Client will be reflected in this Builder.
+         */
+        @SuppressFBWarnings(value = "EI_EXPOSE_REP2", justification = "Pass mutability into wrapping client")
         public Builder wrappedClient(S3Client wrappedClient) {
             this._wrappedClient = wrappedClient;
             return this;

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

@@ -1,12 +1,15 @@
 package software.amazon.encryption.s3.internal;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
+
 import javax.crypto.BadPaddingException;
 import javax.crypto.Cipher;
 import javax.crypto.IllegalBlockSizeException;
@@ -27,8 +30,8 @@
 public class BufferedAesGcmContentStrategy implements ContentEncryptionStrategy, ContentDecryptionStrategy {
 
     // 64MiB ought to be enough for most usecases
-    private final long BUFFERED_MAX_CONTENT_LENGTH_MiB = 64;
-    private final long BUFFERED_MAX_CONTENT_LENGTH_BYTES = 1024 * 1024 * BUFFERED_MAX_CONTENT_LENGTH_MiB;
+    private static final long BUFFERED_MAX_CONTENT_LENGTH_MiB = 64;
+    private static final long BUFFERED_MAX_CONTENT_LENGTH_BYTES = 1024 * 1024 * BUFFERED_MAX_CONTENT_LENGTH_MiB;
 
     final private SecureRandom _secureRandom;
 
@@ -116,6 +119,11 @@ public static class Builder {
 
         private Builder() {}
 
+        /**
+         * Note that this does NOT create a defensive copy of the SecureRandom object. Any modifications to the
+         * object will be reflected in this Builder.
+         */
+        @SuppressFBWarnings(value = "EI_EXPOSE_REP")
         public Builder secureRandom(SecureRandom secureRandom) {
             _secureRandom = secureRandom;
             return this;

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

@@ -1,7 +1,10 @@
 package software.amazon.encryption.s3.internal;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.util.Collections;
 import java.util.Map;
+
 import software.amazon.encryption.s3.algorithms.AlgorithmSuite;
 import software.amazon.encryption.s3.materials.EncryptedDataKey;
 
@@ -45,12 +48,21 @@ public String encryptedDataKeyAlgorithm() {
         return _encryptedDataKeyAlgorithm;
     }
 
+    /**
+     * Note that the underlying implementation uses a Collections.unmodifiableMap which is
+     * immutable.
+     */
+    @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "False positive; underlying"
+        + " implementation is immutable")
     public Map<String, String> encryptedDataKeyContext() {
         return _encryptedDataKeyContext;
     }
 
     public byte[] contentNonce() {
-        return _contentNonce;
+        if (_contentNonce == null) {
+            return null;
+        }
+        return _contentNonce.clone();
     }
 
     public String contentCipher() {

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

@@ -56,7 +56,7 @@ public PutObjectRequest encodeMetadata(EncryptionMaterials materials,
                 EncryptedContent encryptedContent, PutObjectRequest request) {
             Map<String,String> metadata = new HashMap<>(request.metadata());
             EncryptedDataKey edk = materials.encryptedDataKeys().get(0);
-            metadata.put(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2, ENCODER.encodeToString(edk.ciphertext()));
+            metadata.put(MetadataKeyConstants.ENCRYPTED_DATA_KEY_V2, ENCODER.encodeToString(edk.encryptedDatakey()));
             metadata.put(MetadataKeyConstants.CONTENT_NONCE, ENCODER.encodeToString(encryptedContent.nonce));
             metadata.put(MetadataKeyConstants.CONTENT_CIPHER, materials.algorithmSuite().cipherName());
             metadata.put(MetadataKeyConstants.CONTENT_CIPHER_TAG_LENGTH, Integer.toString(materials.algorithmSuite().cipherTagLengthBits()));
@@ -135,7 +135,7 @@ private static ContentMetadata readFromMap(Map<String, String> metadata) {
 
         // Build encrypted data key
         EncryptedDataKey edk = EncryptedDataKey.builder()
-                .ciphertext(edkCiphertext)
+                .encryptedDataKey(edkCiphertext)
                 .keyProviderId(keyProviderId)
                 .keyProviderInfo(keyProviderInfo.getBytes(StandardCharsets.UTF_8))
                 .build();

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

@@ -1,5 +1,7 @@
 package software.amazon.encryption.s3.internal;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -106,6 +108,11 @@ public static class Builder {
 
         private Builder() {}
 
+        /**
+         * Note that this does NOT create a defensive clone of S3Client. Any modifications made to the wrapped
+         * S3Client will be reflected in this Builder.
+         */
+        @SuppressFBWarnings(value = "EI_EXPOSE_REP2", justification = "Pass mutability into wrapping client")
         public Builder s3Client(S3Client s3Client) {
             this._s3Client = s3Client;
             return this;

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

@@ -1,5 +1,7 @@
 package software.amazon.encryption.s3.internal;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.io.IOException;
 
 import software.amazon.awssdk.core.sync.RequestBody;
@@ -60,6 +62,11 @@ public static class Builder {
 
         private Builder() {}
 
+        /**
+         * Note that this does NOT create a defensive clone of S3Client. Any modifications made to the wrapped
+         * S3Client will be reflected in this Builder.
+         */
+        @SuppressFBWarnings(value = "EI_EXPOSE_REP2", justification = "Pass mutability into wrapping client")
         public Builder s3Client(S3Client s3Client) {
             this._s3Client = s3Client;
             return this;

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

@@ -1,8 +1,11 @@
 package software.amazon.encryption.s3.materials;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+
 import software.amazon.awssdk.services.s3.model.GetObjectRequest;
 import software.amazon.encryption.s3.algorithms.AlgorithmSuite;
 
@@ -34,10 +37,22 @@ public AlgorithmSuite algorithmSuite() {
         return _algorithmSuite;
     }
 
+    /**
+     * Note that the underlying implementation uses a Collections.unmodifiableList which is
+     * immutable.
+     */
+    @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "False positive; underlying"
+        + " implementation is immutable")
     public List<EncryptedDataKey> encryptedDataKeys() {
         return _encryptedDataKeys;
     }
 
+    /**
+     * Note that the underlying implementation uses a Collections.unmodifiableMap which is
+     * immutable.
+     */
+    @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "False positive; underlying"
+        + " implementation is immutable")
     public Map<String, String> encryptionContext() {
         return _encryptionContext;
     }

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

@@ -1,9 +1,13 @@
 package software.amazon.encryption.s3.materials;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.util.Collections;
 import java.util.Map;
+
 import javax.crypto.SecretKey;
 import javax.crypto.spec.SecretKeySpec;
+
 import software.amazon.awssdk.services.s3.model.GetObjectRequest;
 import software.amazon.encryption.s3.algorithms.AlgorithmSuite;
 
@@ -43,12 +47,21 @@ public AlgorithmSuite algorithmSuite() {
         return _algorithmSuite;
     }
 
+    /**
+     * Note that the underlying implementation uses a Collections.unmodifiableMap which is
+     * immutable.
+     */
+    @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "False positive; underlying"
+        + " implementation is immutable")
     public Map<String, String> encryptionContext() {
         return _encryptionContext;
     }
 
     public byte[] plaintextDataKey() {
-        return _plaintextDataKey;
+        if (_plaintextDataKey == null) {
+            return null;
+        }
+        return _plaintextDataKey.clone();
     }
 
     public SecretKey dataKey() {