Resolve intermittent getAsInputStream close issue

* Resolve getAsInputStream close issue

* put things back in order and only read this.hmac.getMacSize() bytes

* put the HMAC debug log back where it was

* comment about retry bytes check and retry bytes addition check

* Revert CTR-only fixes

* Fixes #250 and adds unit tests.

* Rename context value names to conform to other names

* Commit failing test cases

* Rename hmacBytes context values and add expected size since multiple digest sizes are possible

* Update CHANGELOG for #254 and #250

CHANGELOG.md

@@ -8,6 +8,12 @@ This project aims to adhere to [Semantic Versioning](http://semver.org/).
    This fix optionally allows users to recursively create a destination directory 
    structure on move(). This is useful for supporting third-party APIs/libraries
    that use this pattern due to a S3 first design. 
+ - Resolve issue where
+   [MantaClient.getAsInputStream().close() throws MantaIOException](https://github.com/joyent/java-manta/issues/250)
+   for encrypted objects with trailing HMAC. Infrequently an issue could occur where
+   less bytes than requested were read and the stream was left in a bad state.
+   This would prevent the underlying connection from being returned to the connection pool.
+ - Exception contexts now [include SDK version.](https://github.com/joyent/java-manta/issues/254)
 
 ## [3.1.0] - 2017-06-07
 

java-manta-client/src/main/java/com/joyent/manta/client/crypto/MantaEncryptedObjectInputStream.java

@@ -652,6 +652,50 @@ private int calculateBufferSize() {
         return bufferSize;
     }
 
+    /**
+     * Reads the HMAC from the end of the underlying stream and returns it as
+     * a byte array.
+     *
+     * @return HMAC as byte array
+     * @throws MantaIOException thrown when stream is unavailable or invalid
+     */
+    private byte[] readHmacFromEndOfStream() throws MantaIOException {
+        final InputStream stream = super.getBackingStream();
+        final int hmacSize = this.hmac.getMacSize();
+        final byte[] hmacBytes = new byte[hmacSize];
+
+        int totalBytesRead = 0;
+        int bytesRead;
+
+        while (totalBytesRead < hmacSize) {
+            final int lengthToRead = hmacSize - totalBytesRead;
+
+            try {
+                 bytesRead = stream.read(hmacBytes, totalBytesRead, lengthToRead);
+            } catch (IOException e) {
+                String msg = "Unable to read HMAC from the end of stream";
+                MantaIOException mioe = new MantaIOException(msg);
+                annotateException(mioe);
+                mioe.setContextValue("backingStreamClass", stream.getClass());
+                mioe.setContextValue("hmacBytesReadTotal", totalBytesRead);
+                mioe.setContextValue("hmacBytesExpected", hmacSize);
+
+                throw mioe;
+            }
+
+            if (totalBytesRead < hmacSize && bytesRead == EOF) {
+                String msg = "No HMAC was stored at the end of the stream";
+                MantaIOException e = new MantaIOException(msg);
+                annotateException(e);
+                throw e;
+            }
+
+            totalBytesRead += bytesRead;
+        }
+
+        return hmacBytes;
+    }
+
     @Override
     public void close() throws IOException {
         synchronized (this.closeLock) {
@@ -664,34 +708,21 @@ public void close() throws IOException {
 
         readRemainingBytes();
 
-        IOUtils.closeQuietly(cipherInputStream);
+        try {
+            IOUtils.closeQuietly(cipherInputStream);
+        } catch (Exception e) {
+            LOGGER.warn("Error closing CipherInputStream", e);
+        }
 
         if (hmac != null && authenticateCiphertext) {
             byte[] checksum = new byte[hmac.getMacSize()];
             hmac.doFinal(checksum, 0);
-            byte[] expected = new byte[this.hmac.getMacSize()];
-            int readHmacBytes = super.getBackingStream().read(expected);
-
-            if (super.getBackingStream().read() != EOF) {
-                String msg = "Expecting the end of the stream. However, more "
-                        + "bytes were available.";
-                MantaIOException e = new MantaIOException(msg);
-                annotateException(e);
-                throw e;
-            }
+            byte[] expected = readHmacFromEndOfStream();
 
             if (LOGGER.isDebugEnabled()) {
                 LOGGER.debug("Calculated HMAC is: {}", Hex.encodeHexString(checksum));
             }
 
-            if (readHmacBytes != this.hmac.getMacSize()) {
-                MantaIOException e = new MantaIOException("The HMAC stored was in the incorrect size");
-                annotateException(e);
-                e.setContextValue("expectedHmacSize", this.hmac.getMacSize());
-                e.setContextValue("actualHmacSize", readHmacBytes);
-                throw e;
-            }
-
             if (super.getBackingStream().read() >= 0) {
                 MantaIOException e = new MantaIOException("More bytes were available than the "
                         + "expected HMAC length");

java-manta-client/src/test/java/com/joyent/manta/client/crypto/IncompleteByteReadInputStream.java

@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2017, Joyent, Inc. All rights reserved.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+package com.joyent.manta.client.crypto;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+/**
+ * {@link java.io.InputStream} implementation that will always read one byte
+ * less than the length specified in a {@link InputStream#read(byte[], int, int)}
+ * method invocation.
+ *
+ * @author <a href="https://github.com/dekobon">Elijah Zupancic</a>
+ * @since 3.1.1
+ */
+public class IncompleteByteReadInputStream extends InputStream {
+    private final InputStream wrapped;
+
+    public IncompleteByteReadInputStream(final InputStream wrapped) {
+        this.wrapped = wrapped;
+    }
+
+    @Override
+    public int read() throws IOException {
+        return wrapped.read();
+    }
+
+    @Override
+    public int read(final byte[] b) throws IOException {
+        return read(b, 0, b.length);
+    }
+
+    @Override
+    public int read(byte[] b, int off, int len) throws IOException {
+        if (len > 1 && len - off - 1 > 0) {
+            return wrapped.read(b, off, len - 1);
+        } else {
+            return wrapped.read(b, off, len);
+        }
+    }
+}

java-manta-client/src/test/java/com/joyent/manta/client/crypto/MantaEncryptedObjectInputStreamTest.java

@@ -11,10 +11,13 @@
 import com.joyent.manta.client.MantaObjectInputStream;
 import com.joyent.manta.client.MantaObjectResponse;
 import com.joyent.manta.exception.MantaClientEncryptionCiphertextAuthenticationException;
+import com.joyent.manta.exception.MantaIOException;
 import com.joyent.manta.http.MantaHttpHeaders;
 import com.joyent.manta.http.entity.MantaInputStreamEntity;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.IOUtils;
+import org.apache.commons.io.input.BoundedInputStream;
+import org.apache.commons.io.output.NullOutputStream;
 import org.apache.commons.lang3.RandomUtils;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.conn.EofSensorInputStream;
@@ -32,6 +35,7 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
 import java.io.RandomAccessFile;
 import java.io.UncheckedIOException;
 import java.net.URISyntaxException;
@@ -570,6 +574,64 @@ public void canCopyStreamWithLargeBufferBufferUnauthenticatedAesCtr256() throws
     }
 
 
+
+    public void willErrorIfCiphertextIsMissingHmacAesCbc128() throws IOException {
+        willErrorWhenMissingHMAC(AesCbcCipherDetails.INSTANCE_128_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willErrorIfCiphertextIsMissingHmacAesCbc192() throws IOException {
+        willErrorWhenMissingHMAC(AesCbcCipherDetails.INSTANCE_192_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willErrorIfCiphertextIsMissingHmacAesCbc256() throws IOException {
+        willErrorWhenMissingHMAC(AesCbcCipherDetails.INSTANCE_256_BIT);
+    }
+
+    public void willErrorIfCiphertextIsMissingHmacAesCtr128() throws IOException {
+        willErrorWhenMissingHMAC(AesCtrCipherDetails.INSTANCE_128_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willErrorIfCiphertextIsMissingHmacAesCtr192() throws IOException {
+        willErrorWhenMissingHMAC(AesCtrCipherDetails.INSTANCE_192_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willErrorIfCiphertextIsMissingHmacAesCtr256() throws IOException {
+        willErrorWhenMissingHMAC(AesCtrCipherDetails.INSTANCE_256_BIT);
+    }
+
+
+    public void willValidateIfHmacIsReadInMultipleReadsAesCtr128() throws IOException {
+        willValidateIfHmacIsReadInMultipleReads(AesCtrCipherDetails.INSTANCE_128_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willValidateIfHmacIsReadInMultipleReadsAesCtr192() throws IOException {
+        willValidateIfHmacIsReadInMultipleReads(AesCtrCipherDetails.INSTANCE_192_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willValidateIfHmacIsReadInMultipleReadsAesCtr256() throws IOException {
+        willValidateIfHmacIsReadInMultipleReads(AesCtrCipherDetails.INSTANCE_256_BIT);
+    }
+
+    public void willValidateIfHmacIsReadInMultipleReadsAesCbc128() throws IOException {
+        willValidateIfHmacIsReadInMultipleReads(AesCbcCipherDetails.INSTANCE_128_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willValidateIfHmacIsReadInMultipleReadsAesCbc192() throws IOException {
+        willValidateIfHmacIsReadInMultipleReads(AesCbcCipherDetails.INSTANCE_192_BIT);
+    }
+
+    @Test(groups = {"unlimited-crypto"})
+    public void willValidateIfHmacIsReadInMultipleReadsAesCbc256() throws IOException {
+        willValidateIfHmacIsReadInMultipleReads(AesCbcCipherDetails.INSTANCE_256_BIT);
+    }
+
     /* TEST UTILITY CLASSES */
 
     private static class EncryptedFile {
@@ -1016,6 +1078,51 @@ private void willThrowExceptionWhenCiphertextIsAltered(SupportedCipherDetails ci
         Assert.assertTrue(thrown, "Ciphertext authentication exception wasn't thrown");
     }
 
+    private void willErrorWhenMissingHMAC(final SupportedCipherDetails cipherDetails) throws IOException {
+        SecretKey key = SecretKeyUtils.generate(cipherDetails);
+        EncryptedFile encryptedFile = encryptedFile(key, cipherDetails, this.plaintextSize);
+        long ciphertextSize = encryptedFile.file.length();
+        int hmacSize = cipherDetails.getAuthenticationTagOrHmacLengthInBytes();
+        long ciphertextSizeWithoutHmac = ciphertextSize - hmacSize;
+
+        boolean thrown = false;
+
+        try (FileInputStream fin = new FileInputStream(encryptedFile.file);
+             BoundedInputStream in = new BoundedInputStream(fin, ciphertextSizeWithoutHmac);
+             MantaEncryptedObjectInputStream min = createEncryptedObjectInputStream(key, in,
+                     ciphertextSize, cipherDetails, encryptedFile.cipher.getIV(),
+                     true, (long)this.plaintextSize)) {
+            // Do a single read to make sure that everything is working
+            Assert.assertNotEquals(min.read(), -1,
+                    "The encrypted stream should not be empty");
+        } catch (MantaIOException e) {
+            if (e.getMessage().startsWith("No HMAC was stored at the end of the stream")) {
+                thrown = true;
+            } else {
+                throw e;
+            }
+        }
+
+        Assert.assertTrue(thrown, "Expected MantaIOException was not thrown");
+    }
+
+    private void willValidateIfHmacIsReadInMultipleReads(final SupportedCipherDetails cipherDetails)
+            throws IOException {
+        SecretKey key = SecretKeyUtils.generate(cipherDetails);
+        EncryptedFile encryptedFile = encryptedFile(key, cipherDetails, this.plaintextSize);
+        long ciphertextSize = encryptedFile.file.length();
+
+        try (FileInputStream fin = new FileInputStream(encryptedFile.file);
+             IncompleteByteReadInputStream ibrin = new IncompleteByteReadInputStream(fin);
+             MantaEncryptedObjectInputStream min = createEncryptedObjectInputStream(key, ibrin,
+                     ciphertextSize, cipherDetails, encryptedFile.cipher.getIV(),
+                     true, (long)this.plaintextSize);
+             OutputStream out = new NullOutputStream()) {
+
+            IOUtils.copy(min, out);
+        }
+    }
+
     private EncryptedFile encryptedFile(
             SecretKey key, SupportedCipherDetails cipherDetails,
             long plaintextSize) throws IOException {