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. |
This branch has not been deployed
2 participants
Rationale for this change
Fsspec uses S3
DeleteObjectswhen deleting files, including a single file. The REST signer currently omits the XML body, so the signing service cannot authorize the object keys in the request.Include the UTF-8 body for
POST ?delete, matching the Java client's behavior. Other request bodies remain untouched.Are these changes tested?
Yes. The three new
DeleteObjectscases failed before the fix because the signing request had nobody. Six more cases check that other methods and queries, including?undeleteand?key=delete, neither forward nor read the request body.Are there any user-facing changes?
REST signing requests for S3 bulk deletion now include the object list required for authorization.