Repository navigation
Fix REST signing for S3 bulk deletes - #4039
smaheshwar-pltr wants to merge 1 commit into
Conversation
smaheshwar-pltr
left a comment
smaheshwar-pltr
left a comment
There was a problem hiding this comment.
The change follows the Java signer's DeleteObjects handling. The inline notes link the protocol contract, the Python serialization path, and the caller that makes this necessary.
| "uri": request.url, | ||
| "headers": {key: [val] for key, val in request.headers.items()}, | ||
| } | ||
| if request.method == "POST" and "delete" in parse_qs(urlparse(request.url).query, keep_blank_values=True): |
There was a problem hiding this comment.
This matches the Java client's existing predicate and body handling: POST with a delete query key. The REST contract calls out DeleteObjects because its body contains the keys that the signing service must validate; the URL identifies only the bucket. keep_blank_values=True makes both ?delete and ?delete= recognizable. Other POST operations, including multipart completion, keep their existing behavior.
| } | ||
| if request.method == "POST" and "delete" in parse_qs(urlparse(request.url).query, keep_blank_values=True): | ||
| if body := request.body: | ||
| signer_body["body"] = body.decode("utf-8") |
There was a problem hiding this comment.
The bytes assumption comes from Botocore's REST XML serializer, which serializes the request using its UTF-8 default. AWSRequest.body exposes that prepared body. Decoding creates the string expected by the signing API; it does not replace the original XML bytes or checksum headers sent to S3. The guards above prevent accessing unrelated upload streams.
|
|
||
|
|
||
| @pytest.mark.parametrize("query", ["delete", "delete=", "delete=&x-id=DeleteObjects"]) | ||
| def test_s3v4_rest_signer_sends_delete_objects_body(requests_mock: Mocker, query: str) -> None: |
There was a problem hiding this comment.
This is reachable even when deleting one file: FsspecFileIO.delete calls fs.rm, and s3fs batches those paths through _bulk_delete, which calls DeleteObjects. All three cases here failed before the fix because the captured signing JSON had no body. The six cases below separately guard against forwarding or reading unrelated request bodies.
|
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. |
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>
5d96c0e to
cd46cb3
Compare
This branch has not been deployed
2 participants
Rationale for this change
FsspecFileIO.deletegoes through s3fs, which sends aDeleteObjectsrequest even for a single file. That request lists the object keys in its XML body, and the URL only names the bucket.S3V4RestSignernever forwarded the body, so a signing service could not check which objects it was authorizing. The REST spec'sbodyfield on the sign request exists for this case.The signer now forwards the body when botocore's
before-signevent names theDeleteObjectsoperation. The Java client checks forPOSTwith adeletequery parameter instead. botocore already passes the operation name to signing hooks, so this avoids parsing the URL. Other requests are unchanged, so upload bodies are never read or sent to the signing service.Are these changes tested?
Yes. A new
s3test writes, reads, and deletes a file throughS3V4RestSigneragainst the local S3 store used bymake test-s3and theintegration-test-s3CI job. The signing endpoint is arequests_mockstub that SigV4-signs each request with the store's credentials, so every S3 call goes through the REST signing path. The test checks that exactly one sign request carries a body and that it names the deleted key. It fails without the fix.Are there any user-facing changes?
Sign requests for
DeleteObjectsnow include the request body.🤖 Generated with Claude Code