Fix #284 MPU Automatic Retry #286

tjcelaya · 2017-07-13T00:30:27Z

For #284:

…aded successfully

…ctionUtils

…ntaReflectionUtils" This reverts commit 1e5d633.

tjcelaya · 2017-07-14T01:04:09Z

Closing this PR until it's actually ready for review.

tjcelaya · 2017-07-14T19:00:44Z

Reopening PR for visibility into ongoing work.

…o fix version inconsistencies

cburroughs

I'm still trying to wrap my head around everything, so I'm afraid I'll need more than one pass.

I know the changes are spread across a few different classes but somewhere there should be a Big Theory Statement with a few paragraphs of prose explaining how this fits together.
Eventually we will need a CHANGELOG entry. It might be helpful to write that now, to explain how this interacts with NSS and what cases we are committing to work (and equally important, which will not work).
What do we need to do either with tests here or on the CI side to make sure this continues to work in the future.
Do any of the existing microbenchmarks cover an interaction that includes record? if so, is there a change? if not, some should be added to get a sense of how expensive an operation this is.
I believe some of the integration make strong assumptions about the handling of validated part numbers. Do they all still pass so far?

cburroughs · 2017-07-20T19:28:17Z

java-manta-client/pom.xml

@@ -170,6 +170,20 @@
            <version>${dependency.urlbuilder.version}</version>
        </dependency>

+        <!-- object cloning library for memoization of encryption state -->


Can we include a "golden file" to deserialize as part of the kyro tests? That way we can be sure that version X objects can still be read if we bump the library dep to X+1.

Regarding shading/relocating. I think the stance that java-manta has taken so far is to do so wherever possible, so I would start with that as the default position.

cburroughs · 2017-07-20T19:29:26Z

java-manta-client/pom.xml

@@ -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>


There are a few different deep cloneing libraries, any reason why this one in particular?

The readme says "Also cloning of files and streams might make the JVM crash." Are Files or Streams among the things we are cloning?

I couldn't find any other libraries for deep-cloning besides this one and the copy functionality provided by kryo. Utilizing kryo would effectively mean bringing java-manta-client-kryo-serialization into the main module.

I've limited usage of this library to only cloning Cipher objects produced by the BouncyCastle library. The HmacCloner and DigestCloner classes in this PR leverage the Memoable interface and some reflection to clone Hmac objects instead.

cburroughs · 2017-07-20T19:32:25Z

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

@@ -89,7 +89,7 @@ public EncryptingPartEntity(final OutputStream cipherStream,

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


What does isRepeatable actually do? If we never "repeat" how is the retry and snapshot code triggered?

The underlying Apache HTTP Client library uses this method to determine if it can automatically retry a request. MantaHttpRequestRetryHandler is only responsible for determining whether a request should be retried based on the exception thrown. The HttpEntity also gets to decide whether it is eligible for automatic retries by returning a value here. See HttpClient source for more context

Having this be true or delegate to the wrapped instance means that the underlying library could transparently retry the request. While it might be possible to automatically reset Cipher state if the MantaHttpRequestRetryHandler was used to coordinate the reset, we'd be adding a lot of complexity that we don't need right now. (This actually sounds like a possibility but enabling automatic retries to work in concert with encryption is not within the scope of this PR. Might tackle that later.)

Since uploadPart already throws IOException users know any call might fail and should be prepared to retry that part. Snapshots only exist in the scope of EncryptedMultipartManager#uploadPart and act as a sort of momentarily-available checkpoint that should never be visible to the user.

cburroughs · 2017-07-20T19:34:27Z

...-manta-client/src/main/java/com/joyent/manta/client/multipart/EncryptedMultipartManager.java

+     * Object used to reset the state of encryption/authentication in case
+     * a part needs to be reuploaded.
+     */
+    private static final EncryptionStateRecorder RECORDER = new EncryptionStateRecorder();


Why static?

EncryptionStateRecorder is a helper class and has no state of its own. This reference has been removed and the nature of the class has been clarified by newer commits (private constructor, only static methods)

cburroughs · 2017-07-20T19:42:53Z

...-manta-client/src/main/java/com/joyent/manta/client/multipart/EncryptedMultipartManager.java

        } finally {
+            if (encryptionState.getLastPartNumber() != partNumber && snapshot != null) {


Is the rewind done for any error during wrapped.uploadPart? Previously the part ordering validation effectively kept consumers from shooting themselves in the foot with retrying parts. I'm trying to figure out of this opes up any new error paths.

Using an inner try now but yes I'm hesitant to classify exceptions by their likelihood of having affected Cipher state an only rewind in certain cases, especially given the narrow scope of the inner try block. It seems much more straightforward to always reset encryption state if we don't make it to the setLastPartNumber invocation.

cburroughs · 2017-07-20T19:43:37Z

...-manta-client/src/main/java/com/joyent/manta/client/multipart/EncryptedMultipartManager.java

        } finally {
+            if (encryptionState.getLastPartNumber() != partNumber && snapshot != null) {
+                // a snapshot was prepared but we didn't make it past uploadPart, rewind EncryptionState
+                RECORDER.rewind(encryptionState, snapshot);


Can this possibly throw an exception? I'm worried about anything happening in the same finally as the lock release.

Moved into an inner try block to address this.

cburroughs · 2017-07-20T19:46:33Z

java-manta-client/src/main/java/com/joyent/manta/client/multipart/EncryptionStateRecorder.java

+            try {
+                writeField(FIELD_HMACOUTPUTSTREAM_HMAC, digestStream, snapshot.getHmac());
+            } catch (IllegalAccessException e) {
+                throw new MantaReflectionException("Failed to overwrite HmacOutputStream's hmac", e);


Is there any additional context or IDs from EncryptionState that we can add to this and the following exceptions?

Changed rewind to accept the EncryptedMultipartUpload object instead to it can include the ID in addition to the part number in the exception messages.

cburroughs · 2017-07-20T19:52:00Z

java-manta-it/src/test/java/com/joyent/test/util/FailingInputStream.java

+
+    private void failAfterMinimum(final int next) throws SpuriousIOException {
+        if (count.get() + next > minimumBytes) {
+            throw new SpuriousIOException("Random error");


nit: This is and arbitrary but not "random" error. It might be nice to include which byte the error was induced at in the message.

tjcelaya · 2017-07-20T22:53:38Z

Problem

The current design of MPU operations involves creating a new Cipher object and, in cases where the desired algorithm does not provide authentication, a new HMac object. These objects are then bound to a CipherOutputStream and an HmacOutputStream which are used to perform encryption and authentication respectively. Any data written to these streams (during a call to uploadPart) changes their state. In the case of a network failure, the state of these objects will be altered by the partially-uploaded content. The diagram below illustrates this situation, the first half of part2's data would be considered part of the plaintext when an automatic retry is triggered:

  x = network failure

  Normal operation:
  
   iv part1 part2
  |-- ----- -----|

  Automatic Retry:

   iv part1 part2 part2
  |-- ----- --x   -----|

Proposal

We can remedy this situation by taking snapshots of Cipher and Hmac states between parts and restoring those snapshots if we encounter an exception during uploadPart. Snapshots are recorded by cloning the Cipher and Hmac objects which sit inside of their respective OutputStreams, both of which are extracted and written to using reflection. We expect the following stream hierarchy (the cloned objects are indicated with <---):

  EncryptionState
    OutputStream cipherStream:
      HmacOutputStream
        HMac <---
        OutputStream out:
          CipherOutputStream extends FilterOutputStream (BouncyCastle)
            Cipher <---
            OutputStream out:
              CloneShieldOutputStream (used to allow writing the HMAC after the ciphertext)
                OutputStream out:
                  MultipartOutputStream (used to allow attaching to http socket output)

Limitations

Unfortunately the implementation of EncryptionStateRecorder is heavily coupled to the way EncryptionState's cipherStream field is constructed but I don't see a better way to accomplish this without adding package-private methods to EncryptionState, EncryptionContext, HmacOutputStream and CipherOutputStream, the last of which is actually provided by an external library and would need to be reimplemented. There was some investigation into creating ProxyCipher and ProxyHmac objects but that didn't seem feasible. I've added warnings to makeCipherOutputForStream about the coupling with EncryptionStateRecorder.

By cloning only the Cipher and HMac objects we can avoid worrying about extracting and replacing streams and focus on copying and swapping in only the objects that hold necessary state.

tjcelaya · 2017-07-21T01:29:08Z

Answering general questions from @cburroughs

I know the changes are spread across a few different classes but somewhere there should be a Big Theory Statement with a few paragraphs of prose explaining how this fits together.
- Added a comment to this PR which outlines the problem, proposed solution, and limitations
Eventually we will need a CHANGELOG entry. It might be helpful to write that now, to explain how this interacts with NSS and what cases we are committing to work (and equally important, which will not work).
- Added fix to CHANGELOG in this PR
What do we need to do either with tests here or on the CI side to make sure this continues to work in the future.
- I added a testcase EncryptedServerSideMultipartManagerIT#canRetryUploadPart which exercises this behavior withint the context of a full MPU. I'm put together a test which exercises just EncryptionStateRecorder today, but its usefulness outside the context of uploadPart is dubious
Do any of the existing microbenchmarks cover an interaction that includes record? if so, is there a change? if not, some should be added to get a sense of how expensive an operation this is.
- None of the benchmarks address the modified code, though I also can't figure out how the current benchmarks actually work.. I used jvisualvm to profile the cloning process and found the most expensive portion was the cloning of the Provider class referenced by Cipher. Using dontCloneInstanceOf cut the time taken for cloning from ~250ms to ~15ms but it makes sense to automate this measurement.
I believe some of the integration make strong assumptions about the handling of validated part numbers. Do they all still pass so far?
- The integration test suite passed when I ran it this morning, though I've made a few changes today. The change in part number is actually used as the indication of whether or not the call to uploadPart was successful. The inner try block which was introduced as a result of your comments is now very tightly scoped to only surround uploadPart and setLastPartNumber.

…ach combination of cipher mode and strength

tjcelaya · 2017-07-21T19:06:43Z

@cburroughs @dekobon Ready for a second round of reviews. I would specifically appreciate review of the way the objenisis and cloning libraries are shaded.

dekobon

LGTM

dekobon · 2017-07-21T21:59:00Z

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

+        HmacSHA512
+    }
+
+    /**


This isn't a package default constructor. This is a private constructor.

dekobon · 2017-07-21T21:59:59Z

java-manta-client/src/main/java/com/joyent/manta/client/multipart/EncryptionStateRecorder.java

@@ -0,0 +1,176 @@
+/*
+ * Copyright (c) 2016-2017, Joyent, Inc. All rights reserved.


Change copyright to 2017.

tjcelaya added 11 commits July 12, 2017 17:13

unfinished test case

c4e00ee

FailingInputStream allows us to provoke a failure in uploadPart

251a0a2

Only call encryptionState.setLastPartNumber(int) if the part was uplo…

d300bb8

…aded successfully

make test actually fail

841ab5b

move HMAC names into an enum

1327f87

move ReflectionUtils into the client package and rename to MantaRefle…

1e5d633

…ctionUtils

checkstyle

3976ec3

stash changes momentarily

5cce1c3

need more tests

a8ae7ee

all the supported HMacs are Memoable..

851a5b2

reduce visiblity of cloners and check directly for Memoable

5a01fc0

tjcelaya changed the title ~~Fix 284 MPU Automatic Retry~~ Fix #284 MPU Automatic Retry Jul 13, 2017

tjcelaya added 5 commits July 12, 2017 17:55

Make AbstractCloner actually abstract

dead6f1

fix subclasses

b9c4910

fix DigestCloner usage

af6866c

save broken test

9152629

Revert "move ReflectionUtils into the client package and rename to Ma…

8f6b0a5

…ntaReflectionUtils" This reverts commit 1e5d633.

tjcelaya closed this Jul 14, 2017

tjcelaya added 4 commits July 14, 2017 11:42

bring back MantaReflectionException

6f2b601

try cloner library

39410ff

CipherCloner fixes

f470d3a

Cloner refactoring with overwrite

4258712

tjcelaya reopened this Jul 14, 2017

tjcelaya added 5 commits July 14, 2017 13:09

create EncryptionStateRewinder

df13a1c

stash potential deadend

1602f48

remove cloning library

794ef7e

CipherSerializerTest prototype

1c7852b

upgrade objenesis version and add an exclusion from the kryo module t…

c421bd7

…o fix version inconsistencies

tjcelaya added 2 commits July 19, 2017 17:07

Pull memento object into its own class

86772b3

stash documentation changes

f383e18

tjcelaya requested a review from cburroughs July 20, 2017 17:36

tjcelaya added 2 commits July 20, 2017 10:39

phrasing

c3dcecb

Merge branch 'master' into fix/284-MPU-automatic-retry

e2e98f3

cburroughs reviewed Jul 20, 2017

View reviewed changes

tjcelaya added 4 commits July 20, 2017 13:30

Update CHANGELOG.md

0a854f2

be more explicit about interaction with libnss

c41b2e1

Add explanation about package-private and static modifiers

c23f275

javadoc updates with more warnings

c549452

tjcelaya added 4 commits July 20, 2017 15:55

hide EncryptionStateRecorder constructor

c67fc84

checkstyle for utility class

6d2df1b

add upload ID and part number to exceptions

afed41e

shade cloning and objenesis

2b7645b

improve CHANGELOG entry

db9af9c

tjcelaya force-pushed the fix/284-MPU-automatic-retry branch from 3ebe286 to ffbfe2c Compare July 21, 2017 04:14

add MPU upload ID to the snapshot

1e35c3c

tjcelaya force-pushed the fix/284-MPU-automatic-retry branch from ffbfe2c to 1e35c3c Compare July 21, 2017 04:15

tjcelaya added 2 commits July 21, 2017 09:07

collapse tests so we dont have to keep around two sets of tests for e…

2b3aef9

…ach combination of cipher mode and strength

partial EncryptionStateRecorder test

67f6940

clarify coverage comment

6baaeaf

dekobon approved these changes Jul 21, 2017

View reviewed changes

fix javadoc in SupportedHmacsLookupMap

5b5184a

tjcelaya merged commit a3f3507 into TritonDataCenter:master Jul 21, 2017

tjcelaya deleted the fix/284-MPU-automatic-retry branch July 21, 2017 23:38

tjcelaya mentioned this pull request Jul 27, 2017

Fix #290 #291

Merged

tjcelaya mentioned this pull request Jan 5, 2018

Automatic retry of encrypted MPU uploads #396

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #284 MPU Automatic Retry #286

Fix #284 MPU Automatic Retry #286

tjcelaya commented Jul 13, 2017 •

edited

Loading

tjcelaya commented Jul 14, 2017

tjcelaya commented Jul 14, 2017

cburroughs left a comment

cburroughs Jul 20, 2017

cburroughs Jul 20, 2017

tjcelaya Jul 20, 2017

cburroughs Jul 20, 2017

tjcelaya Jul 20, 2017 •

edited

Loading

cburroughs Jul 20, 2017

tjcelaya Jul 20, 2017

cburroughs Jul 20, 2017

tjcelaya Jul 20, 2017

cburroughs Jul 20, 2017

tjcelaya Jul 20, 2017

cburroughs Jul 20, 2017

tjcelaya Jul 20, 2017 •

edited

Loading

cburroughs Jul 20, 2017

tjcelaya commented Jul 20, 2017 •

edited

Loading

tjcelaya commented Jul 21, 2017 •

edited

Loading

tjcelaya commented Jul 21, 2017

dekobon left a comment

dekobon Jul 21, 2017

dekobon Jul 21, 2017

		} finally {
		if (encryptionState.getLastPartNumber() != partNumber && snapshot != null) {

		@@ -0,0 +1,176 @@
		/*
		* Copyright (c) 2016-2017, Joyent, Inc. All rights reserved.

Fix #284 MPU Automatic Retry #286

Fix #284 MPU Automatic Retry #286

Conversation

tjcelaya commented Jul 13, 2017 • edited Loading

tjcelaya commented Jul 14, 2017

tjcelaya commented Jul 14, 2017

cburroughs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjcelaya Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjcelaya Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjcelaya commented Jul 20, 2017 • edited Loading

Problem

Proposal

Limitations

tjcelaya commented Jul 21, 2017 • edited Loading

tjcelaya commented Jul 21, 2017

dekobon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjcelaya commented Jul 13, 2017 •

edited

Loading

tjcelaya Jul 20, 2017 •

edited

Loading

tjcelaya Jul 20, 2017 •

edited

Loading

tjcelaya commented Jul 20, 2017 •

edited

Loading

tjcelaya commented Jul 21, 2017 •

edited

Loading