[
https://issues.apache.org/jira/browse/HDDS-17630?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135895#comment-18135895
]
Ritesh Shukla commented on HDDS-17630:
--------------------------------------
Corrected the description after re-checking it against master at ef63bacef7,
this time also on a mini cluster with the AWS SDK for Java v2. A PUT with a
non-empty {{uploadId}} and no {{partNumber}} is already refused on a real
cluster, with 400 InvalidArgument from the part-number check in Ozone's client
library; the 404 in the earlier table came from the unit tests' client stub.
Since HDDS-17498, the server-side encryption check runs before the gateway
picks the operation, so a malformed PUT that also asks for encryption the
gateway cannot honour now fails with 501 and keeps the object; the cited line
numbers also moved. The Testing section now describes the tests as written, and
the lost metadata and tags, the last-part-wins case and the POST and GET
answers on OM were run rather than read or inferred.
----
🤖 Generated with [Claude Code|https://claude.com/claude-code]
> S3G: a part upload with an empty or missing uploadId overwrites the object
> instead of failing
> ---------------------------------------------------------------------------------------------
>
> Key: HDDS-17630
> URL: https://issues.apache.org/jira/browse/HDDS-17630
> Project: Apache Ozone
> Issue Type: Bug
> Components: S3
> Reporter: Ritesh Shukla
> Assignee: Ritesh Shukla
> Priority: Major
>
> h3. Problem
> An S3 {{PUT}} on an object key means one of two things:
> * {{PUT /bucket/key?partNumber=N&uploadId=<id>}} uploads one part of a
> multipart upload (UploadPart). Nothing under the key changes until the upload
> is completed.
> * {{PUT /bucket/key}} writes the object (PutObject).
> When {{uploadId}} is empty, or {{partNumber}} comes with no {{uploadId}} at
> all, the S3 Gateway handles the request as a PutObject, or as a CopyObject
> when {{x-amz-copy-source}} is set. The body (or the copy source) replaces the
> committed object, and the client gets {{200 OK}}, just as for a part upload:
> {code}
> PUT /bucket/key body "original object" -> 200, key
> holds "original object"
> PUT /bucket/key?partNumber=1&uploadId= body "part 1" -> 200, key
> now holds "part 1"
> {code}
> AWS S3 refuses each of these requests and leaves the object alone (tested
> against AWS S3 on 2026-10-31). The Ozone column was run on master
> (ef63bacef7) against a mini cluster with the AWS SDK for Java v2, and in the
> S3 Gateway unit-test harness:
> || Request || AWS S3 || S3 Gateway on master ||
> | {{PUT ?partNumber=1&uploadId=}} | 400 InvalidArgument, object kept | 200,
> object replaced by the body |
> | {{PUT ?partNumber=1&uploadId}} (no {{=}}) | 400 InvalidArgument, object
> kept | 200, object replaced by the body: the gateway reads a bare
> {{uploadId}} as the same empty value |
> | {{PUT ?partNumber=1}} (no {{uploadId}}) | 400 InvalidArgument, object kept
> | 200, object replaced by the body |
> | {{PUT ?partNumber=1&uploadId=}} with {{x-amz-copy-source}} (UploadPartCopy)
> | 400 InvalidArgument, object kept | 200, object replaced by a full copy of
> the source; {{x-amz-copy-source-range}} is ignored |
> | {{PUT ?uploadId=}} (no {{partNumber}}) | 405 MethodNotAllowed, object kept
> | 200, object replaced by the body |
> | {{PUT ?partNumber=}} or {{?partNumber=0}} (no {{uploadId}}), or
> {{?partNumber=&uploadId=}} | 400 InvalidArgument, object kept
> ({{?partNumber=abc}} too) | 200, object replaced by the body |
> | {{PUT ?partNumber=1&uploadId=}} while an upload is in progress on the key |
> 400 InvalidArgument, object kept, no part added | 200, object replaced by the
> body |
> | {{PUT ?uploadId=<non-empty ID>}} (no {{partNumber}}), unknown or real
> upload | 405 MethodNotAllowed, object kept, no part added | 400
> InvalidArgument "Part number must be an integer between 1 and 10000,
> inclusive", object kept, no part added: the gateway reads the missing part
> number as 0, and Ozone's client library in the gateway refuses it before
> calling OM |
> S3's InvalidArgument message is "This operation does not accept partNumber
> without uploadId", for any value of {{partNumber}}. With an empty body, the
> gateway truncates the object to 0 bytes (run).
> On master, the server-side encryption check added by HDDS-17498 runs before
> the gateway picks the operation. So a request that also asks for encryption
> the gateway cannot honour (SSE-C in any bucket, or SSE-S3 or SSE-KMS on an
> unencrypted bucket) is refused first with 501 NotImplemented and keeps the
> object (run for SSE-C and SSE-S3 on an unencrypted bucket, the other cases
> read from code). Without an encryption header, or with one an encrypted
> bucket can honour, the object is replaced as above (run).
> Other handlers that read {{uploadId}} (gateway answers run against a mini
> cluster):
> * {{POST ?uploadId=}} (CompleteMultipartUpload) and {{GET ?uploadId=}}
> (ListParts): S3 and the gateway both answer 404 NoSuchUpload.
> * {{DELETE ?uploadId=}}: S3 answers 404 NoSuchUpload; the gateway deletes the
> object. That is HDDS-17599.
> * {{HEAD ?uploadId=}} and {{GET ?partNumber=1&uploadId=}}: S3 answers 405
> MethodNotAllowed; the gateway answers 200 (HeadObject) and 404 NoSuchUpload.
> Both are read-only and are left out of this fix.
> h3. Impact
> * Only a caller who may already write the key can do this. The operation that
> actually runs is authorized as itself: PutObject, or GetObject on the source
> and PutObject on the destination for the copy form. No permission boundary is
> crossed (read from code).
> * What is lost: the object's data, metadata and tags (run). The response is a
> success, so the client cannot tell. A client that uploads several parts under
> the same empty ID ends up with only the last part as the object (run).
> * How a client gets here: an application bug that passes an empty upload ID,
> for example after losing the CreateMultipartUpload response. Stock clients
> send it as-is. The AWS SDK for Java v2 (2.55.6) and v1 (1.12.788), and the
> AWS CLI v2 (2.35.10), all put {{uploadId=}} on the wire for an empty ID,
> including for UploadPartCopy; both SDKs refuse a null ID before sending (run
> against a local capture server).
> h3. Cause
> {{ObjectEndpoint#handlePutRequest}} treats a request as a part upload only
> when {{uploadID != null && !uploadID.equals("")}} (ObjectEndpoint.java:216 at
> ef63bacef7). Every other request becomes a PutObject, or a CopyObject when
> {{x-amz-copy-source}} is set (ObjectEndpoint.java:238-245). That path never
> reads {{partNumber}}. The only checks that run before this condition are the
> bucket lookup and the server-side encryption check
> (ObjectEndpoint.java:210-212).
> The empty-string test dates from the first UploadPart code (HDDS-902). The
> parameter was then declared with {{@DefaultValue("")}}, so a missing
> {{uploadId}} arrived as an empty string, and comparing with {{""}} was how
> "absent" was written. HDDS-14209 switched to reading query parameters
> directly, where a missing {{uploadId}} is {{null}}, but kept the comparison.
> So a present but empty {{uploadId}} still counts as absent.
> h3. Proposed fix
> Answer as S3 does (tested against AWS S3 on 2026-10-31), before any OM call:
> * {{partNumber}} present and {{uploadId}} missing or empty: 400
> {{InvalidArgument}} with S3's message, "This operation does not accept
> partNumber without uploadId". This also covers UploadPartCopy.
> * {{uploadId}} present, empty or not, and no {{partNumber}}: 405
> {{MethodNotAllowed}}, as S3 answers both. {{S3ErrorTable}} gains a
> {{METHOD_NOT_ALLOWED}} entry with S3's code and message.
> * Everything else is unchanged: a non-empty {{uploadId}} with a
> {{partNumber}} is still a part upload, and a PUT with neither parameter is
> still a PutObject.
> Both checks go at the top of {{handlePutRequest}}, before the bucket lookup
> and the server-side encryption check, so a refused request makes no OM call
> and the key is never opened. It is audited and counted as a failed PutObject.
> The part-upload branch then only needs {{uploadID != null}}.
> The obvious one-line fix, dropping {{!uploadID.equals("")}} so the empty ID
> reaches OM, is worse. Run against a mini cluster, it answers the empty ID
> with OM's 404 NoSuchUpload, not S3's 400, and {{PUT ?partNumber=1}} with no
> {{uploadId}} still overwrites the object. UploadPartCopy would also read the
> source key before failing (read from code). HDDS-17599 answers DELETE with
> NoSuchUpload because that is what S3 does for DELETE; S3 answers PUT
> differently, so this fix does too.
> h3. Testing
> * Unit, {{TestPartUpload}}: a PUT with {{partNumber}} and no non-empty
> {{uploadId}} ({{?partNumber=1&uploadId=}}, {{?partNumber=1}},
> {{?partNumber=}}, {{?partNumber=0}}, {{?partNumber=abc}},
> {{?partNumber=&uploadId=}}) expects S3's 400; a PUT with {{uploadId}} and no
> {{partNumber}} ({{?uploadId=}}, {{?uploadId=<unknown>}}) expects S3's 405;
> and with an upload in progress on the key, {{?partNumber=1&uploadId=}} (400)
> and {{?uploadId=<that upload>}} (405) must add no part. Each test puts a key
> first and checks that its content is unchanged. They run with and without
> data stream. On master all 20 runs fail: 16 replace the key, 2 answer 404
> instead of 405, and 2 are accepted by the unit tests' client stub, which does
> not check part numbers.
> * Unit, {{TestMultipartUploadWithCopy}}: UploadPartCopy with
> {{?partNumber=1&uploadId=}} and with {{?partNumber=1}}. On master both copy
> the source over the destination.
> * {{EndpointTestUtils#put}} stops adding {{partNumber=0}} to plain PUTs and
> clears a left-over {{uploadId}}. A real PutObject carries neither, and the
> new checks would refuse it: with the fix but without this change, 275
> existing unit-test runs in 17 classes fail.
> * Integration, {{AbstractS3SDKV2Tests}}: {{uploadPart}} and
> {{uploadPartCopy}} with {{uploadId("")}}, which the SDK sends as
> {{uploadId=}}. Each expects 400 InvalidArgument with S3's message and the
> object's content unchanged. On master the object is replaced.
> * Run on ef63bacef7 with the change: all 1,943 s3gateway unit-test runs pass,
> and so do the 255 tests of the SDK v1 and v2 integration suite
> ({{TestS3SDK}}). Without the change, only the new tests fail.
> ----
> 🤖 Generated with [Claude Code|https://claude.com/claude-code]
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
