Skip to content

Set outputLength correctly on open ended range requests to encryptedBlobStore#706

Merged
gaul merged 3 commits intogaul:masterfrom
ptemarvelde:fix-encrypted-open-ended-range-requests
Oct 16, 2024
Merged

Set outputLength correctly on open ended range requests to encryptedBlobStore#706
gaul merged 3 commits intogaul:masterfrom
ptemarvelde:fix-encrypted-open-ended-range-requests

Conversation

@ptemarvelde
Copy link
Contributor

@ptemarvelde ptemarvelde commented Oct 11, 2024

This resolves #698.

Open ended ranged requests to encrypted files resulted in incorrect Content-Length headers in the response because of a bug in the Decryption class.

In EncryptedBloBStore the length is set to -1 by default. On open-ended ranged GET requests this value is passed to the Decryption constructor, which in turn only sets outputLength if an offset is given without a length, but -1 is used to represent no length given. After this change the outputLength is set correctly in this constructor.

https://github.com/gaul/s3proxy/blob/master/src/main/java/org/gaul/s3proxy/EncryptedBlobStore.java#L375

@gaul The tests pass (locally) but I have not spend enough time with this codebase to reason about whether this change will break anything else (which is currently untested). Is this a reasonable solution for #698?

@gaul
Copy link
Owner

gaul commented Oct 11, 2024

Can you add a unit test that demonstrates this problem? I wonder how does encryption work for range requests? I would imagine that S3Proxy needs to decrypt either the entire object or at least whatever the encryption block size is. CC @FlorinPeter who wrote the original code.

@ptemarvelde
Copy link
Contributor Author

Partial decryption is already extensively tested in EncryptedBlobStoreTest, see testReadPartial, testMultiPartReadPartial and testMultiPartReadTail. Both testMultiPartRead* tests test the situation where no explicit end byte is given.

As confirmed by these tests, the content s3proxy returns for encrypted blobs for any ranged request is correct. Just the ContentLength response header is incorrect in the described case, due to the bug in the Encryption class. I have extended one of the unittests to verify this behavior and show the content length is set correctly now.

Copy link
Owner

@gaul gaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Please fit the nits so I can merge this.

@gaul gaul merged commit 6185f2b into gaul:master Oct 16, 2024
@gaul
Copy link
Owner

gaul commented Oct 16, 2024

Thank you for your contribution and explanation @ptemarvelde! I didn't write this middleware and don't have a good understanding how it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Incorrect Content-Length in Response Headers for Range Requests on Encrypted Files

2 participants