Repository navigation
Send the DeleteObjects body to the S3 REST signer - #4039
smaheshwar-pltr wants to merge 1 commit into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Thanks for raising this @smaheshwar-pltr. I know pre-signed URLs conceptually, but not the details. Maybe @danielcweeks has a free cycle to take a look at this. Can I ask if you have tested this against a S3 instance? One issue we have with the pre-signed url code is that it doesn't run any integration tests again MinIO. |
5d96c0e to
cd46cb3
Compare
s3fs deletes files with DeleteObjects, which lists the object keys in the request body. S3V4RestSigner never forwarded the body, so a signing service could not authorize the keys before signing. Also add an S3 test that writes, reads, and deletes through the REST signer, which previously had only mocked unit tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cd46cb3 to
2a008e3
Compare
smaheshwar-pltr
changed the title
| "headers": {key: [val] for key, val in request.headers.items()}, | ||
| } | ||
| # The signing service needs the DeleteObjects body to authorize the keys being deleted | ||
| if operation_name == "DeleteObjects": |
There was a problem hiding this comment.
Mirroring Iceberg Java, which sends the DeleteObjects body here. See the PR description for motivation.
Java identifies DeleteObjects from the URL (POST with a delete query parameter). botocore already passes operation_name to every before-sign handler, so the signer now accepts it and checks it directly, with no URL parsing.
|
Hey @Fokko, good to see you again! Thanks for taking a look, I've now opened this PR up for review so things should be clearer. In response to #4039 (comment), we're dealing with remote signing here not pre-signed URLs, and I've since added an integration test against RustFS where the signing endpoint is a mock that lets us verify the the body is as expected. To me, this seems sufficient. I've also updated the PR description to better describe the problem we're seeing currently 🙏 |
This branch has not been deployed
2 participants
Rationale for this change
Iceberg Java sends the
DeleteObjectsbody to the S3 signing service, as the REST spec says to. PyIceberg doesn't. The body is the only place the keys being deleted appear, so REST catalogs like Lakekeeper rejectDeleteObjectssign requests without it.Since
FsspecFileIO.deletesendsDeleteObjectseven for a single file, PyIceberg can't delete files through remote signing with these catalogs today.Are these changes tested?
Yes. A new
s3test writes, reads, and deletes a file throughS3V4RestSigneragainst the S3 container that theintegration-test-s3CI job runs. Arequests_mocksigning endpoint SigV4-signs each request with the container's credentials. The test checks that only theDeleteObjectssign request carries a body, and it fails without the fix.Are there any user-facing changes?
Sign requests for
DeleteObjectsnow include the request body.AI Disclosure: This PR was authored by Claude Code (Opus 5.5) but directed and overseen by me.