Skip to content

Commit

Permalink
Fix #284 MPU Automatic Retry (#286)
Browse files Browse the repository at this point in the history
When using encryption in combination with Multipart Uploads, automatic retries triggered by the underlying HTTP library could cause file corruption. In case of a network error automatically certain requests would be retried transparently (e.g. those backed by `File`s and `byte[]` data). This caused authentication and encryption state erroneously include the partial content from the initial request. As a result of this fix encrypted MPU operations will utilize the BouncyCastle cryptography library even when libnss (PKCS#11) is installed.
  • Loading branch information
tjcelaya authored Jul 21, 2017
1 parent bb1a00f commit a3f3507
Show file tree
Hide file tree
Showing 29 changed files with 1,114 additions and 24 deletions.
10 changes: 8 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@
All notable changes to this project will be documented in this file.
This project aims to adhere to [Semantic Versioning](http://semver.org/).


## [3.x.x] - XXXX-XX-XX
### Changed
- The heuristics for guessing heuristics have been adjusted to give
- The heuristics for guessing Content-Type have been adjusted to give
[more consistent](https://github.com/joyent/java-manta/issues/276)
results across platforms.
### Fixed
- When using encryption in combination with Multipart Uploads,
[automatic retries](https://github.com/joyent/java-manta/issues/284) triggered by the underlying HTTP
library could cause file corruption. In case of a network error automatically certain requests would be retried
transparently (e.g. those backed by `File`s and `byte[]` data). This caused authentication and encryption state
erroneously include the partial content from the initial request. As a result of this fix encrypted MPU operations
will utilize the BouncyCastle cryptography library even when libnss (PKCS#11) is installed.

## [3.1.3] - 2017-06-29
### Fixed
Expand Down
11 changes: 4 additions & 7 deletions java-manta-client-kryo-serialization/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@
<groupId>com.esotericsoftware</groupId>
<artifactId>minlog</artifactId>
</exclusion>
<exclusion>
<groupId>org.objenesis</groupId>
<artifactId>objenesis</artifactId>
</exclusion>
</exclusions>
</dependency>

Expand All @@ -116,13 +120,6 @@
</exclusions>
</dependency>

<dependency>
<groupId>org.objenesis</groupId>
<artifactId>objenesis</artifactId>
<version>${dependency.objenesis.version}</version>
<scope>compile</scope>
</dependency>

<!-- These dependencies are declared at the module level because we can not
inherit exclusions from the parent. -->
<dependency>
Expand Down
22 changes: 22 additions & 0 deletions java-manta-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,20 @@
<version>${dependency.urlbuilder.version}</version>
</dependency>

<!-- object cloning library for memoization of encryption state -->
<dependency>
<groupId>uk.com.robust-it</groupId>
<artifactId>cloning</artifactId>
<version>${dependency.cloning.version}</version>
</dependency>

<!-- object instantiation library, needed by uk.com.robust-it:cloning and the kryo module -->
<dependency>
<groupId>org.objenesis</groupId>
<artifactId>objenesis</artifactId>
<version>${dependency.objenesis.version}</version>
</dependency>

<!-- These dependencies are declared at the module level because we can not
inherit exclusions from the parent. -->
<dependency>
Expand Down Expand Up @@ -238,6 +252,14 @@
<pattern>io.mikael.urlbuilder</pattern>
<shadedPattern>com.joyent.manta.io.mikael.urlbuilder</shadedPattern>
</relocation>
<relocation>
<pattern>org.objenesis</pattern>
<shadedPattern>com.joyent.manta.org.objenesis</shadedPattern>
</relocation>
<relocation>
<pattern>com.rits</pattern>
<shadedPattern>com.joyent.manta.com.rits</shadedPattern>
</relocation>
</relocations>
<filters>
<!-- explicitly remove class that causes security concerns -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ public Cipher getCipher() {
provider);
}

@Override
public Cipher getBouncyCastleCipher() {
return SupportedCipherDetails.findCipher(cipherAlgorithmJavaName,
ExternalSecurityProviderLoader.getBouncyCastleProvider());
}

@Override
public HMac getAuthenticationHmac() {
if (this.isAEADCipher) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ private EncryptingEntityHelper() {
* used. This allows us to support EtM authentication for ciphers/modes that
* do not natively support authenticating encryption.
*
* NOTE: The design of {@link com.joyent.manta.client.multipart.EncryptionStateRecorder}
* is heavily coupled to this implementation! Changing how these streams are
* wrapped requires changes to EncryptionStateRecorder!
*
* @param httpOut output stream for writing to the HTTP network socket
* @param encryptionContext current encryption running state
* @return a new stream configured based on the parameters
Expand Down Expand Up @@ -74,6 +78,10 @@ public static OutputStream makeCipherOutputForStream(
* used. This allows us to support EtM authentication for ciphers/modes that
* do not natively support authenticating encryption.
*
* NOTE: The design of {@link com.joyent.manta.client.multipart.EncryptionStateRecorder}
* is heavily coupled to this implementation! Changing how these streams are
* wrapped requires changes to EncryptionStateRecorder!
*
* @param httpOut output stream for writing to the HTTP network socket
* @param encryptionContext current encryption running state
* @param hmac current HMAC object with the current checksum state
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public EncryptingPartEntity(final OutputStream cipherStream,

@Override
public boolean isRepeatable() {
return wrapped.isRepeatable();
return false;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public EncryptionContext(final SecretKey key,

this.key = key;
this.cipherDetails = cipherDetails;
this.cipher = cipherDetails.getCipher();
this.cipher = cipherDetails.getBouncyCastleCipher();
initializeCipher();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ public Cipher getCipher() {
return null;
}

@Override
public Cipher getBouncyCastleCipher() {
fail();
return null;
}

@Override
public long ciphertextSize(final long plaintextSize) {
fail();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ public interface SupportedCipherDetails {
*/
Cipher getCipher();

/**
* In some cases we <em>need</em> the Cipher to live entirely in Java.
* See {@link com.joyent.manta.client.multipart.EncryptionStateRecorder}
*
* @return a new instance of the associated cipher using the BouncyCastle provider
*/
Cipher getBouncyCastleCipher();

/**
* Calculates the size of the output ciphertext based on the plaintext
* size.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public final class SupportedHmacsLookupMap extends LookupMap<String, Supplier<HM
public static final SupportedHmacsLookupMap INSTANCE = new SupportedHmacsLookupMap();

/**
* Package default constructor because interface is through {@link SupportedCipherDetails}.
* Private constructor because interface is through {@link SupportedCipherDetails}.
*/
private SupportedHmacsLookupMap() {
super(MantaUtils.unmodifiableMap(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ protected MantaMultipartUploadPart uploadPart(final EncryptedMultipartUpload<WRA
encryptionState.getMultipartStream(), encryptionContext));
}

final EncryptionStateSnapshot snapshot = EncryptionStateRecorder.record(encryptionState, upload.getId());
final EncryptingPartEntity entity = new EncryptingPartEntity(
encryptionState.getCipherStream(),
encryptionState.getMultipartStream(), sourceEntity,
Expand All @@ -277,8 +278,18 @@ public ByteArrayOutputStream call(final long uploadedBytes) throws IOException {
}
}
});
encryptionState.setLastPartNumber(partNumber);
return wrapped.uploadPart(upload.getWrapped(), partNumber, entity);

try {
final MantaMultipartUploadPart part = wrapped.uploadPart(upload.getWrapped(), partNumber, entity);
encryptionState.setLastPartNumber(partNumber);
return part;
} catch (Exception e) {
if (encryptionState.getLastPartNumber() != partNumber) {
// didn't make it to encryptionState.setLastPartNumber(partNumber)
EncryptionStateRecorder.rewind(encryptionState, snapshot);
}
throw e;
}
} finally {
encryptionState.getLock().unlock();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ public EncryptedMultipartUpload(final WRAPPED wrapped,
final EncryptionState encryptionState) {
this.wrapped = wrapped;
this.encryptionState = encryptionState;

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import java.util.concurrent.locks.ReentrantLock;

/**
* Package private class that contains the state of the encryption streaming
* Class that contains the state of the encryption streaming
* ciphers used to encrypt multipart uploads.
*
* @author <a href="https://github.com/cburroughs/">Chris Burroughs</a>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
/*
* 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.multipart;

import com.joyent.manta.client.crypto.EncryptionContext;
import com.joyent.manta.exception.MantaReflectionException;
import com.joyent.manta.util.CipherCloner;
import com.joyent.manta.util.Cloner;
import com.joyent.manta.util.HmacCloner;
import com.joyent.manta.util.HmacOutputStream;
import org.apache.commons.lang3.Validate;
import org.bouncycastle.crypto.macs.HMac;
import org.bouncycastle.jcajce.io.CipherOutputStream;

import javax.crypto.Cipher;
import java.io.OutputStream;
import java.lang.reflect.Field;
import java.util.UUID;

import static org.apache.commons.lang3.reflect.FieldUtils.getField;
import static org.apache.commons.lang3.reflect.FieldUtils.readField;
import static org.apache.commons.lang3.reflect.FieldUtils.writeField;

/**
* Helper class for recording state of encryption and authentication state during in-progress MPU uploads.
* Used to enable retry of {@link EncryptedMultipartManager#uploadPart} in case of network failure.
* Like the <a href="https://sourcemaking.com/design_patterns/memento">Memento Pattern</a> but with less respect
* for encapsulation.
*
* This class and its methods are package-private because they rely on locking provided by EncryptionState and need
* to be called a specific points during the MPU process. This class holds none of its own state as as a result
* all methods have been marked as {@code static}
*/
final class EncryptionStateRecorder {

private EncryptionStateRecorder() {
}

/**
* Reference to {@link HmacOutputStream}'s {@link HMac} field.
*/
private static final Field FIELD_HMACOUTPUTSTREAM_HMAC = getField(HmacOutputStream.class, "hmac", true);

/**
* Reference to {@link HmacOutputStream}'s {@link OutputStream} field.
*/
private static final Field FIELD_HMACOUTPUTSTREAM_OUT = getField(HmacOutputStream.class, "out", true);

/**
* Reference to {@link EncryptionContext}'s {@link Cipher} field.
*/
private static final Field FIELD_ENCRYPTIONCONTEXT_CIPHER = getField(EncryptionContext.class, "cipher", true);

/**
* Reference to {@link CipherOutputStream}'s {@link Cipher} field.
*/
private static final Field FIELD_CIPHEROUTPUTSTREAM_CIPHER = getField(CipherOutputStream.class, "cipher", true);

/**
* {@link HMac} cloning helper object.
*/
private static final Cloner<HMac> CLONER_HMAC = new HmacCloner();

/**
* {@link Cipher} cloning helper object.
*/
private static final Cloner<Cipher> CLONER_CIPHER = new CipherCloner();

/**
* Make sure the wrapping stream performs an HMAC digest and cast to the needed type.
*
* @param cipherStream the encrypting stream which we are verifying is wrapped in an HMac digest
* @return the HmacOutputStream
*/
private static HmacOutputStream ensureHmacWrapsCipherStream(final OutputStream cipherStream) {
if (!cipherStream.getClass().equals(HmacOutputStream.class)) {
final String message = "Cipher lacks authentication but OutputStream is not HmacOutputStream";
throw new IllegalStateException(message);
}

return (HmacOutputStream) cipherStream;
}

/**
* Clones Cipher (and potentially HMAC) instances for future use.
* This should be called before data is streamed in uploadPart.
*/
static EncryptionStateSnapshot record(final EncryptionState encryptionState, final UUID uploadId) {
final HMac hmac;

if (!encryptionState.getEncryptionContext().getCipherDetails().isAEADCipher()) {
OutputStream cipherStream = encryptionState.getCipherStream();
final HmacOutputStream digestStream = ensureHmacWrapsCipherStream(cipherStream);
hmac = CLONER_HMAC.createClone(digestStream.getHmac());
} else {
hmac = null;
}

final Cipher cipher =
CLONER_CIPHER.createClone(encryptionState.getEncryptionContext().getCipher());

return new EncryptionStateSnapshot(uploadId, encryptionState.getLastPartNumber(), cipher, hmac);
}

/**
* Restore Cipher (and potentially HMAC) instances to a provided snapshot. We want to avoid
* producing a new EncryptionState for a few reasons:
*
* 1. The {@code ReentrantLock} in {@code EncryptionState} is used to synchronize access
* to internal encryption state, including the creation and restoration of snapshots (record and rewind).
*
* 2. Interaction between CipherOutputStream and HmacOutputStream
* makes it non-trivial construct a copy of an EncryptionState that is completely separate from the original.
*/
static void rewind(final EncryptionState encryptionState, final EncryptionStateSnapshot snapshot) {
Validate.notNull(snapshot.getCipher(),
"Snapshot cipher must not be null");
Validate.isTrue(encryptionState.getLastPartNumber() == snapshot.getLastPartNumber(),
"Snapshot part number must equal encryption state part number");
final boolean usesHmac = !encryptionState.getEncryptionContext().getCipherDetails().isAEADCipher();

final CipherOutputStream cipherStream;
if (usesHmac) {
Validate.notNull(snapshot.getHmac(), "Snapshot hmac must not be null");

final HmacOutputStream digestStream = ensureHmacWrapsCipherStream(encryptionState.getCipherStream());

try {
writeField(FIELD_HMACOUTPUTSTREAM_HMAC, digestStream, snapshot.getHmac());
} catch (IllegalAccessException e) {
final String message = String.format("Failed to overwrite HmacOutputStream's hmac while rewinding "
+ "encryption state for upload [%s] part [%s]",
snapshot.getUploadId(),
snapshot.getLastPartNumber());
throw new MantaReflectionException(message, e);
}

Object wrappedCipherStream;
try {
wrappedCipherStream = readField(FIELD_HMACOUTPUTSTREAM_OUT, digestStream);
} catch (IllegalAccessException e) {
final String message = String.format("Failed to extract wrapped OutputStream while rewinding "
+ "encryption state for upload [%s] part [%s]",
snapshot.getUploadId(),
snapshot.getLastPartNumber());
throw new MantaReflectionException(message, e);
}

if (!(wrappedCipherStream instanceof CipherOutputStream)) {
final String message = String.format("Expected HmacOutputStream to wrap CipherOutputStream "
+ "while rewinding encryption state for upload [%s] part [%s], found %s",
snapshot.getUploadId(),
snapshot.getLastPartNumber(),
wrappedCipherStream.getClass().getCanonicalName());
throw new MantaReflectionException(message);
}

cipherStream = (CipherOutputStream) wrappedCipherStream;
} else {
cipherStream = (CipherOutputStream) encryptionState.getCipherStream();
}

try {
writeField(FIELD_ENCRYPTIONCONTEXT_CIPHER, encryptionState.getEncryptionContext(), snapshot.getCipher());
writeField(FIELD_CIPHEROUTPUTSTREAM_CIPHER, cipherStream, snapshot.getCipher());
} catch (IllegalAccessException e) {
final String message = String.format("Failed to overwrite cipher while rewinding "
+ "encryption state for upload [%s] part [%s]",
snapshot.getUploadId(),
snapshot.getLastPartNumber());
throw new MantaReflectionException(message, e);
}
}
}
Loading

0 comments on commit a3f3507

Please sign in to comment.