[
https://issues.apache.org/jira/browse/HDDS-17631?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135898#comment-18135898
]
Ritesh Shukla commented on HDDS-17631:
--------------------------------------
Re-checked on master at ef63bacef7: every single-valued element in the five
request models is still read as its last occurrence, and new unit tests and a
mini-cluster test fail on master for that reason. Corrected the proposed fix:
two TagSet elements in a bucket tagging body stay accepted, because the AWS SDK
for Java 1.x writes one TagSet per tag set and S3's answer to that body is not
known, and PutBucketAcl repeats now wait for an S3 test. The namespace rows now
say which of them need HDDS-17632, and the MalformedXML mapping and the shared
model walk now point to HDDS-17600 (apache/ozone#11860), which provides both.
The description also adds And and AbortIncompleteMultipartUpload to the
repeated containers, and says which rows were run through the gateway's
handlers and which were read from code.
----
🤖 Generated with [Claude Code|https://claude.com/claude-code]
> S3G: request XML that repeats a single-valued element is accepted and read as
> its last occurrence
> -------------------------------------------------------------------------------------------------
>
> Key: HDDS-17631
> URL: https://issues.apache.org/jira/browse/HDDS-17631
> Project: Apache Ozone
> Issue Type: Bug
> Components: S3
> Reporter: Ritesh Shukla
> Assignee: Ritesh Shukla
> Priority: Major
>
> h3. Problem
> An S3 request body names each setting once. In a multi-object delete each
> {{Object}} has one {{Key}}. A tag has one {{Key}} and one {{Value}}. A
> multipart {{Part}} has one {{PartNumber}} and one {{ETag}}. An ACL {{Grant}}
> has one {{Permission}}, and a lifecycle {{Rule}} has one {{Prefix}},
> {{Status}} and {{Expiration}}. Only lists repeat: {{Object}}, {{Part}},
> {{Tag}} inside {{TagSet}} or {{And}}, {{Grant}}, {{Rule}} and {{Transition}}.
> When a body gives a single-valued element twice, the S3 Gateway accepts it
> and silently uses the last one. AWS S3 refused every such body it was sent
> with 400 {{MalformedXML}} and changed nothing (tested against AWS S3 on
> 2026-10-31).
> For example, this DeleteObjects body names two keys for one object:
> {code:xml}
> <Delete><Object><Key>tmp/a</Key><Key>secret</Key></Object></Delete>
> {code}
> The gateway deletes {{secret}}. S3 answers 400 {{MalformedXML}} and deletes
> nothing. A standard XML reader asked for the object's key returns {{tmp/a}}:
> Python's ElementTree {{find("Key")}} and the DOM's
> {{getElementsByTagName("Key").item(0)}} both do. So one request means
> different things to different readers, and the gateway picks one without
> saying so.
> The same happens in every request that has an XML body:
> ||Given twice in the body||Gateway on master||AWS S3, 2026-10-31||
> |DeleteObjects {{Key}}: {{tmp/a}}, then {{secret}}|deletes {{secret}}|400
> {{MalformedXML}}, nothing deleted|
> |DeleteObjects {{Quiet}}: {{true}}, then {{false}}|uses {{false}}|400
> {{MalformedXML}}, in either order|
> |PutObjectTagging {{Value}}: {{a}}, then {{b}}|stores {{b}}|400
> {{MalformedXML}}, no tag stored|
> |PutObjectTagging {{Key}}: {{k1}}, then {{k2}}|stores {{k2}}|400
> {{MalformedXML}}, no tag stored|
> |CompleteMultipartUpload {{PartNumber}}: {{12}}, then {{1}}|completes with
> part 1|400 {{MalformedXML}}|
> |CompleteMultipartUpload {{ETag}}: part 12's, then part 1's|uses part 1's|400
> {{MalformedXML}}|
> |{{<Key>tmp/a</Key><Key xmlns="urn:other">secret</Key>}}|deletes
> {{secret}}|400 {{MalformedXML}}|
> |{{<s3:Key>tmp/a</s3:Key><Key>secret</Key>}}, with {{s3}} bound to the S3
> namespace|deletes {{secret}}|400 {{MalformedXML}}|
> |{{<Key>tmp/a</Key><x:Key>secret</x:Key>}}, with {{x}} bound to another
> namespace|deletes {{tmp/a}}|400 {{MalformedXML}}|
> |PutBucketAcl {{Permission}}: {{READ}}, then {{FULL_CONTROL}}|grants
> {{FULL_CONTROL}}|not tested|
> |PutBucketLifecycleConfiguration {{Prefix}}: {{safe/}}, then
> {{secret/}}|expires objects under {{secret/}}|not tested|
> |PutBucketLifecycleConfiguration {{Days}}: {{3650}}, then {{1}}|expires
> objects after 1 day|not tested|
> S3 ignores a change of default namespace and reads an element whose prefix is
> bound to the S3 namespace as the plain element, so the first two namespace
> rows hold two {{Key}} elements as far as S3 is concerned. An element whose
> prefix is bound to another namespace is refused by S3 even on its own
> (tested), so the third row is refused either way. The gateway's parser does
> not process namespaces, so this fix alone refuses the first namespace row.
> The second row is refused once HDDS-17632 resolves prefixes, and the third
> row is HDDS-17632's alone. ACL and lifecycle bodies were not sent to S3, to
> leave the test bucket's settings alone. PutBucketTagging reads the same model
> as PutObjectTagging; only PutObjectTagging was sent to S3.
> A repeated container behaves the same way on the gateway. A second
> {{TagSet}}, {{Owner}}, {{AccessControlList}}, {{Grantee}}, {{Expiration}},
> {{AbortIncompleteMultipartUpload}}, {{Filter}} or {{And}}, or a second
> {{Tag}} directly under a lifecycle {{Filter}}, replaces the first whole;
> nothing is merged. A later empty element wins too: {{<Key>tmp/a</Key><Key/>}}
> is read as an empty key.
> How the gateway column was produced: each body was run on master (ef63bacef7)
> through the gateway's own unmarshaller classes, in a standalone harness with
> jaxb-runtime 2.3.9 and Hadoop's secure SAX parser factory. The tagging, ACL
> and lifecycle rows were also run through the gateway's handlers in unit
> tests, and the CompleteMultipartUpload row on a mini cluster; each stored or
> completed the last value. Only the delete itself is read from the handler
> code.
> h3. Impact
> * No common client sends these bodies. The AWS SDK for Java 1.x and the AWS
> CLI write each single-valued element once (checked by serializing all six
> requests offline). Hand-built XML can repeat them.
> * One SDK path does repeat a container. The AWS SDK for Java 1.x accepts
> several tag sets in a bucket tagging configuration and writes one {{TagSet}}
> element for each (read from its bytecode and run offline). Today the gateway
> keeps only the last set's tags. S3's answer to that body was not tested, so
> the fix leaves it as it is today rather than refuse a request that a stock
> client sends.
> * Ozone's own permission checks run on the value the gateway acts on, so they
> are not bypassed. This is inferred from the code, not run.
> * The harm is the silent choice. Anything else that reads the same body and
> takes the first value sees a different request from the one Ozone runs. That
> includes an audit trail, a policy check, or a person reviewing a lifecycle
> rule. They would see another key deleted, {{READ}} where {{FULL_CONTROL}} is
> granted, or a 3650-day expiry where Ozone expires data after 1 day.
> h3. Cause
> On master (ef63bacef7), {{MessageUnmarshaller.readFrom}}
> (MessageUnmarshaller.java:74-86) streams the body through
> {{XmlNamespaceFilter}} into JAXB. JAXB stores a single-valued field each time
> its element appears, so the last occurrence wins, and a repeated child object
> replaces the earlier one whole. Nothing counts occurrences.
> {{XmlNamespaceFilter}} (XmlNamespaceFilter.java:43-53) only forces the
> namespace. The lifecycle filter in
> {{PutBucketLifecycleConfigurationUnmarshaller}} (lines 35-47 and 66-75)
> checks which children are allowed, not how many times each appears.
> The affected request models are {{MultiDeleteRequest}},
> {{CompleteMultipartUploadRequest}}, {{S3Tagging}} (object and bucket
> tagging), {{S3BucketAcl}} and {{S3LifecycleConfiguration}}.
> h3. Proposed fix
> Answer as S3 does: refuse a body in which a single-valued element appears
> more than once in the same parent, with 400 {{MalformedXML}}, before anything
> is changed.
> * Take "single-valued" from the request model classes, not from a hand list.
> A {{List}} field may repeat; every other bound field may appear at most once.
> For every element Ozone reads, this matches S3's published API model.
> * One exception until S3's answer is known: a second {{TagSet}} in a tagging
> body stays accepted, because the AWS SDK for Java 1.x sends one {{TagSet}}
> per tag set. If S3 refuses that body, the exception goes; if S3 reads one or
> all of the sets, the gateway should read the same.
> * Count per parent model class, not by element name. A lifecycle {{Tag}} is
> single under {{Filter}} but a list under {{And}}. A rule by name alone would
> either refuse the {{And}} bodies that the SDKs send, or let two {{Tag}}
> elements under {{Filter}} through.
> * Name elements as S3 does when counting. The gateway's parser does not
> process namespaces, so an element in another default namespace already counts
> as the plain element, as it does for S3 (tested 2026-10-31). A prefixed
> element is not recognised at all today; HDDS-17632 resolves prefixes. That
> change must land with or after this one. Without this check, an emulation of
> that resolution reads {{<Key>a</Key><s3:Key>b</s3:Key>}} as {{b}}, last
> occurrence wins, where today the gateway reads {{a}} (run in a harness, not
> sent to S3).
> * Keep accepting elements that Ozone does not model, as today. Newer SDKs
> send some of them, for example {{ChecksumCRC32}} in a {{Part}}.
> * Answer {{MalformedXML}} for all six requests. On master a parse failure
> answers {{InvalidRequest}} for DeleteObjects, CompleteMultipartUpload and
> PutBucketAcl. HDDS-17600 (apache/ozone#11860) answers every request-body
> parse failure with {{MalformedXML}}, so this refusal needs no mapping of its
> own.
> * S3's answer for repeated elements in PutBucketLifecycleConfiguration bodies
> is not tested yet. The fix refuses them the same way. If a later test shows
> that S3 reads one of the values instead, the gateway should read that same
> value for that request.
> * PutBucketAcl waits for an S3 test. Until S3's answer to a repeated element
> in an ACL body is known, the fix leaves ACL bodies as they are today.
> Keeping the first occurrence instead is not better. S3 refuses these bodies,
> and whichever occurrence the gateway picks, some other reader picks the other
> one.
> This check builds on HDDS-17600 (apache/ozone#11860), which works out once
> per request class which elements are values and which hold other elements,
> per parent element, and keeps a stack of the open elements in the namespace
> filter. This change records for each parent which children are {{List}}
> fields, and counts the children of each open container in that same stack.
> h3. Testing
> * Unit, in s3gateway: one parameterized test per request type except
> PutBucketAcl, whose tests wait for S3's answer. It covers every row of the
> table except the two prefixed rows (HDDS-17632) and the PutBucketAcl row,
> plus {{VersionId}}, {{Status}}, {{ID}}, {{Expiration}},
> {{AbortIncompleteMultipartUpload}}, {{Filter}}, {{And}}, {{Tag}} and
> {{Prefix}} under {{Filter}}, {{Prefix}} under {{And}}, the {{Transition}}
> values and {{DaysAfterInitiation}} given twice. Each must answer
> {{MalformedXML}} and change nothing. Run on master (ef63bacef7), each case
> fails because the body is accepted with the last value.
> * Unit, bodies that must stay accepted: several {{Object}}, {{Part}},
> {{Tag}}, {{Grant}}, {{Rule}} and {{Transition}} elements; an {{And}} with two
> {{Tag}} elements; {{Quiet}} between two {{Object}} elements; an unmodelled
> {{ChecksumCRC32}} in a {{Part}}; two {{TagSet}} elements in a bucket tagging
> body, as the AWS SDK for Java 1.x writes them.
> * Existing tests: the planned check, emulated in a standalone harness, was
> run over every request body in the s3gateway unit tests, the S3 SDK
> integration tests, the smoketests and the test resources at ef63bacef7. That
> is 153 bodies, with the parameterized lifecycle cases expanded. It changed no
> result and no error message.
> * Integration: in {{AbstractS3SDKV2Tests}}, a presigned
> CompleteMultipartUpload whose {{Part}} has {{PartNumber}} twice answers 400
> {{MalformedXML}}, and the upload can still be completed afterwards. Run on
> master (ef63bacef7), it answers 200 and completes the upload with the part
> the last {{PartNumber}} names.
> h3. Not covered here
> * S3 also refuses an element that its schema does not define. For example,
> {{<Object><Wrap><Key>tmp/a</Key></Wrap><Key>secret</Key></Object>}} gets 400
> {{MalformedXML}} from S3 (tested 2026-10-31). The gateway ignores {{Wrap}}
> and deletes {{secret}}. Matching that needs S3's list of elements for each
> parent, which the request models do not hold, so it is a separate change.
> * For the same reason, a repeated element that S3 defines but Ozone does not
> read, such as {{ETag}} inside a delete {{Object}}, stays accepted and ignored.
> ----
> 🤖 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]
