Fix #297 #302
Fix #297 #302
Conversation
|
Summary of changes:
|
|
After reviewing with @dekobon the following changes have been requested:
|
…java-manta-client testng.xml
…tream buffer copy
| decryptingStream.close(); | ||
| AssertJUnit.assertArrayEquals(plaintext, decrypted.toByteArray()); | ||
| } | ||
|
|
||
| } |
| @@ -183,7 +183,7 @@ public EncryptionState read(final Kryo kryo, final Input input, final Class<Encr | |||
| final HMac hmac = kryo.readObjectOrNull(input, HMac.class); | |||
|
|
|||
| final OutputStream cipherStream = EncryptingEntityHelper.makeCipherOutputForStream( | |||
| multipartStream, encryptionContext, hmac); | |||
| multipartStream, encryptionContext.getCipherDetails(), encryptionContext.getCipher(), hmac); | |||
There was a problem hiding this comment.
Does changing the class have any compatibility consequences? I don't think we have a clear version compatibility statement for the kyro module.
There was a problem hiding this comment.
I suggest that the java-manta-client and java-manta-client-kryo-serialization must be the same version. We should probably document this.
There was a problem hiding this comment.
Added a compatibility method and a note to the installation section in the README
| @@ -83,12 +88,15 @@ public static OutputStream makeCipherOutputForStream( | |||
| * wrapped requires changes to EncryptionStateRecorder! | |||
| * | |||
| * @param httpOut output stream for writing to the HTTP network socket | |||
| * @param encryptionContext current encryption running state | |||
| * @param cipherDetails current encryption running state's cipher details | |||
| * @param cipher current encryption running state's cipher | |||
There was a problem hiding this comment.
I'm not sure the phrase "current encryption running state's" will make sense to future readers with EncryptionContext no longer being in the signature.
There was a problem hiding this comment.
I'll clarify this.
| final OutputStream httpOut, final EncryptionContext encryptionContext, | ||
| final OutputStream httpOut, | ||
| final SupportedCipherDetails cipherDetails, | ||
| final Cipher cipher, |
There was a problem hiding this comment.
Should cipher be validated for nullness?
There was a problem hiding this comment.
It seems pretty silly to pass a null Cipher to this method, I'll add a call to Validate#notNull
There was a problem hiding this comment.
Not a blocker for this PR but do we want to validate that the hmac is not null when the CipherDetails indicates the cipher is not AEAD?
| * @throws IOException in case of network errors, though MantaMultipartException will be thrown in | ||
| * case of invalid response code | ||
| */ | ||
| private MantaMultipartUploadPart uploadPartSafely(final EncryptedMultipartUpload<WRAPPED_UPLOAD> upload, |
There was a problem hiding this comment.
Minor: I know it's private, but someone is going to skim the javadoc and be alarmed that the other methods appear "unsafe." Maybe uploadPartWithSnapshot?
There was a problem hiding this comment.
Not sure why that name didn't pop into my head last night, I was too tired. Making this change.
| MantaMultipartUploadPart finalPart = wrapped.uploadPart(upload.getWrapped(), | ||
| encryptionState.getLastPartNumber() + 1, | ||
| remainderStream.toByteArray()); | ||
| final MantaMultipartUploadPart finalPart = uploadPartSafely( |
There was a problem hiding this comment.
If this method most always be called inside the encryptionState.getLock(), I think that should get a comment to guard against errant future refactoring.
There was a problem hiding this comment.
Making this change.
| return part; | ||
| } catch (Exception e) { | ||
| if (encryptionState.getLastPartNumber() != partNumber) { | ||
| // didn't make it to encryptionState.setLastPartNumber(partNumber) |
There was a problem hiding this comment.
Is setLastPartNumber the official "point of no return"?
| */ | ||
| private final HMac hmac; |
There was a problem hiding this comment.
Removed because the HMAC will be within the cloned hmaOutputStream, correct?
| return lastPartNumber == that.lastPartNumber | ||
| && cipher == that.cipher | ||
| && hmac == that.hmac; | ||
| return Objects.equals(uploadId, that.uploadId) |
There was a problem hiding this comment.
Sorry if I breezed by on the last patch, why does this class needs equals & hashcode methods.
There was a problem hiding this comment.
They're not strictly necessary but it was a matter of habit. The fact that EncryptionState has those methods was the primary motivation.
|
Sorry for the delay, how the questions help. |
…age of EncryptingEntityHelper and a note in the readme about expectations regarding mixing module versions
…AndLastPartAuth invocation
Created a unit test for the situation where EncryptingPartEntity finalizes encryption. Not passing yet.
(issue #297)