fix: do not truncate encrypted streams when offset is greater than zero #2113
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seebees
requested changes
May 28, 2025
Comment on lines
119
to
121
| // the original offset MUST NOT be part of the length calculation, | ||
| // or else the encryption will be truncated. | ||
| size = len - (offset - off); |
There was a problem hiding this comment.
Suggested change
| // the original offset MUST NOT be part of the length calculation, | |
| // or else the encryption will be truncated. | |
| size = len - (offset - off); | |
| // `offset` is a local variable that accumulates the encrypted data, | |
| // and it is initialized with `off`. | |
| // Therefore the original offset, `off`, MUST be removed | |
| // or else the encryption will be truncated. | |
| size = (len - offset) - off; |
src/test/java/com/amazonaws/encryptionsdk/internal/FrameEncryptionHandlerTest.java
Show resolved
Hide resolved
seebees
reviewed
May 28, 2025
| public void testStreamTruncation() throws Exception { | ||
| String testDataString = StringUtils.repeat("Hello, World! ", 10_000); | ||
|
|
||
| int startOffset = 100; // The data will start from this offset |
There was a problem hiding this comment.
Suggested change
| int startOffset = 100; // The data will start from this offset | |
| int startOffset = 100; // This is the offset we will use for our test |
| inputData, | ||
| startOffset, | ||
| inputData.length - startOffset); | ||
| // decryptData doesn't know about the offset |
There was a problem hiding this comment.
Suggested change
| // decryptData doesn't know about the offset | |
| // decryptData doesn't know about the offset, because we will encrypt starting at `startOffset` |
src/test/java/com/amazonaws/encryptionsdk/internal/FrameEncryptionHandlerTest.java
Show resolved
Hide resolved
| try (ByteArrayOutputStream os = new ByteArrayOutputStream()) { | ||
| try (CryptoOutputStream cryptoOutput = | ||
| crypto.createEncryptingStream(keyring, os, Collections.emptyMap())) { | ||
| cryptoOutput.write(inputData, startOffset, inputData.length - startOffset); |
There was a problem hiding this comment.
Suggested change
| cryptoOutput.write(inputData, startOffset, inputData.length - startOffset); | |
| // Here we use the offset to encrypt only part of the inputData | |
| cryptoOutput.write(inputData, startOffset, inputData.length - startOffset); |
src/test/java/com/amazonaws/encryptionsdk/internal/FrameEncryptionHandlerTest.java
Show resolved
Hide resolved
seebees
approved these changes
May 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #2112
Description of changes: Fixes the truncation bug described in the issue above. I think using parens and subtracting offset is easier to read than the suggested change but is logically the same. We could also introduce a new variable e.g.
bytesProcessedwhich starts at 0 and counts up bysizeeach loop, but I think this approach is most readable.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: