[
https://issues.apache.org/jira/browse/HDDS-17632?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135899#comment-18135899
]
Ritesh Shukla commented on HDDS-17632:
--------------------------------------
Re-checked every claim on master at commit ef63bacef7, with unit tests through
the S3 endpoints and a mini-cluster test, and updated the description. Raised
the priority to Major: a PutBucketAcl body with prefixed Grant elements answers
200 and empties the bucket's ACL (measured on a mini cluster). Results that
were read from code are now measured, and a prefixed AccessControlList is added
as a further failure: a NullPointerException escapes the request. The proposed
fix now says how the resolved name is used and how it combines with HDDS-17631,
its ACL part waits for an S3 test of ACL bodies, and the Testing section lists
the tests that fail on master.
----
🤖 Generated with [Claude Code|https://claude.com/claude-code]
> S3G: prefixed elements in request XML are ignored; PutBucketAcl can empty the
> bucket ACL and DeleteObjects fails
> ----------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17632
> URL: https://issues.apache.org/jira/browse/HDDS-17632
> Project: Apache Ozone
> Issue Type: Bug
> Components: S3
> Reporter: Ritesh Shukla
> Assignee: Ritesh Shukla
> Priority: Minor
>
> h3. Problem
> In XML an element can carry a namespace prefix. {{<s3:Key>}} with
> {{xmlns:s3="http://s3.amazonaws.com/doc/2006-03-01/"}} is the same {{Key}}
> element as an unprefixed {{<Key>}} in the S3 namespace. Tested against AWS S3
> on 2026-10-31, S3 reads an element whose prefix is bound to the S3 namespace,
> or whose prefix is not declared at all, as the plain element: it deletes the
> key named by a prefixed {{Key}}, and accepts DeleteObjects, PutObjectTagging
> and CompleteMultipartUpload bodies in which the elements are prefixed. It
> refuses with 400 {{MalformedXML}} an element whose prefix is bound to another
> namespace, even on its own. It also refused {{a:b:Key}}, whose first part
> {{a}} was bound to another namespace. A change of default namespace
> ({{xmlns="..."}}) is ignored.
> The S3 Gateway matches elements by their raw name, prefix included. To the
> gateway, {{s3:Key}} is an unknown element, so it skips it. It then acts on
> the request as if that element were not there.
> Example. This DeleteObjects body names one key:
> {code:xml}
> <Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/";
> xmlns:s3="http://s3.amazonaws.com/doc/2006-03-01/";>
> <Object><s3:Key>photos/cat.jpg</s3:Key></Object>
> </Delete>
> {code}
> S3 answers 200 and deletes {{photos/cat.jpg}}. The gateway reads an
> {{Object}} without a key. Building the request to OM then fails with a
> NullPointerException. Nothing is deleted, not even the other keys in the same
> request, and the client gets a server error instead of a delete result.
> ||DeleteObjects body||AWS S3 (tested 2026-10-31)||Ozone today||
> |{{<Key>k</Key>}}, with or without {{xmlns}}|200, deletes {{k}}|same|
> |{{<s3:Key>k</s3:Key>}}, {{s3}} bound to the S3 namespace|200, deletes
> {{k}}|reads no key; the request fails and nothing is deleted|
> |{{<s3:Key>k</s3:Key>}}, {{s3}} not declared|200, deletes {{k}}|reads no key;
> the request fails and nothing is deleted|
> |every element prefixed: {{<s3:Delete><s3:Object><s3:Key>k</s3:Key>...}}|200,
> deletes {{k}}|400 {{InvalidRequest}}: "unexpected element ... s3:Delete" (400
> {{MalformedXML}} once HDDS-17600 lands)|
> |{{<x:Key>k</x:Key>}} alone, {{x}} bound to {{urn:other}}|400
> {{MalformedXML}}|reads no key; the request fails and nothing is deleted|
> |{{<a:b:Key>k</a:b:Key>}} (two colons), {{a}} bound to {{urn:a}}|400
> {{MalformedXML}}|reads no key; the request fails and nothing is deleted|
> |{{<s3:Key>a</s3:Key><Key>b</Key>}} in one {{Object}}|400 {{MalformedXML}}:
> {{Key}} given twice|reads and deletes {{b}}|
> |{{<Key>a</Key><x:Key>b</x:Key>}}, {{x}} bound to {{urn:other}}|400
> {{MalformedXML}}|reads and deletes {{a}}|
> |a second {{<Object xmlns="urn:foreign">}}|200, deletes both keys|same|
> |an {{Object}} with no {{Key}}, or with only a {{VersionId}}|400
> {{MalformedXML}}|reads no key; the request fails and nothing is deleted|
> Ozone's answers were measured on master at commit {{ef63bacef7}}: the
> readings by unit tests through the S3 endpoints, and the example on a mini
> cluster through a presigned DeleteObjects request with one plain and one
> prefixed {{Key}}. The gateway answered 500, its log shows the
> NullPointerException from protobuf's {{addAllKeys}}, and both keys still
> existed afterwards.
> The last row is not about prefixes, but it fails the same way: any {{Object}}
> that ends up without a key reaches the NullPointerException below.
> The other request bodies go through the same filter. S3 accepted a fully
> prefixed {{TagSet}} (and stored the tag) and a prefixed {{Part}} (and
> completed the upload); ACL and lifecycle bodies were not sent to S3. Measured
> on the gateway:
> * PutObjectTagging and PutBucketTagging skip a prefixed {{TagSet}} or
> {{Tag}}. The tag set is then missing or empty, and the request is refused
> with 400 {{MalformedXML}}.
> * CompleteMultipartUpload skips prefixed {{Part}} elements. The empty part
> list is refused with 400 {{MalformedXML}}.
> * PutBucketAcl skips prefixed {{Grant}} elements and reads an empty grant
> list. It then replaces the bucket's ACL with that empty list and answers 200.
> On a mini cluster the bucket ACL went from one grant (the creator, user
> {{user}}, all rights) to an empty list.
> * PutBucketAcl with a prefixed {{AccessControlList}} reads no list at all. A
> NullPointerException escapes the endpoint ({{S3Acl.java:212}}). As in the
> DeleteObjects run, that gives the client a server error (not run for this
> body).
> * PutBucketLifecycleConfiguration checks {{s3:LifecycleConfiguration}}
> against its list of allowed elements and refuses the body. The handler
> answers 400 {{MalformedXML}} (from code).
> h3. Impact
> * No client checked is affected. The AWS CLI 2.35.10 and the AWS SDK for Java
> 1.12.788 and 2.55.6 (the version Ozone builds against) never prefix S3
> elements. Their bodies for all five request types were captured and read.
> Each body either sends no namespace or declares the S3 namespace as the
> default namespace; SDK v1 sends no namespace except on ACL bodies. Other
> clients were not checked, and hand-written XML can use prefixes.
> * A client that writes prefixes gets failures where S3 succeeds.
> DeleteObjects fails as a whole, and a fully prefixed body is refused.
> Tagging, multipart completion and lifecycle requests are refused.
> PutBucketAcl is the worst case: it answers 200 and empties the bucket's ACL
> instead of applying the grants (measured). With native ACLs enabled, users
> who had access only through those grants lose it (from code).
> * With a prefixed {{Key}} next to a plain one, the gateway deletes one of the
> two keys where S3 refuses the request. When the prefix is bound to the S3
> namespace, that body also repeats an element; see HDDS-17631.
> * Permissions are not bypassed. OM still checks every key and ACL change
> against the caller's rights (from code).
> h3. Cause
> * {{MessageUnmarshaller.java:55}} gets its parser from
> {{XMLUtils.newSecureSAXParserFactory()}}, which is not namespace-aware. Such
> a parser reports an element only by its raw name, such as {{s3:Key}}, and
> leaves the local name empty.
> * {{XmlNamespaceFilter.java:46}} and {{XmlNamespaceFilter.java:52}} put every
> element in the S3 namespace but pass its names on unchanged. JAXB then
> matches the raw name: for the fully prefixed body, its error reports the
> element {{s3:Delete}}.
> * The request classes bind plain names, such as {{@XmlElement(name = "Key")}}
> at {{MultiDeleteRequest.java:66-67}}. Nothing binds {{s3:Key}}, and JAXB
> skips unknown elements without an error. A prefixed root element matches no
> root, which {{MessageUnmarshaller.java:85}} reports as {{InvalidRequest}}.
> * {{PutBucketLifecycleConfigurationUnmarshaller.java:68}} checks the same raw
> name against its allowed elements.
> * After the missing key (run on a mini cluster; the audit and the exception
> mappers were read in the code): {{endpoint/BucketEndpoint.java:402}} adds it
> to the key list as null.
> {{OzoneManagerProtocolClientSideTranslatorPB.java:1055}} passes the list to
> protobuf's {{addAllKeys}}, which throws a NullPointerException for the null
> entry. No request reaches OM. {{BucketEndpoint.multiDelete}} catches only
> {{IOException}} there ({{endpoint/BucketEndpoint.java:427}}), so the
> exception escapes it with no per-key error and no audit entry, and no
> exception mapper turns it into an S3 error. The client gets HTTP 500.
> * After the empty grant list (measured on a mini cluster):
> {{BucketAclHandler.java:156}} turns it into an empty ACL list, and
> {{BucketAclHandler.java:177}} sets that list on the bucket. OM replaces the
> bucket's ACLs with it ({{OMBucketSetAclRequest.java:70}},
> {{AclListBuilder.java:95}}).
> h3. Proposed fix
> Answer as S3 does (tested 2026-10-31). Keep the parser as it is; the filter
> tracks prefix declarations itself, since the parser reports {{xmlns:p}} as an
> ordinary attribute.
> * Resolve each element name once, before {{XmlNamespaceFilter}} or any other
> check reads it. Keep the prefix declarations ({{xmlns:p}}) of each open
> element; a declaration applies to the element that carries it and to its
> descendants. A prefix bound to the S3 namespace, or not declared, resolves to
> the local name, the part after the colon. Pass the resolved name to JAXB as
> both the local name and the qualified name, and use it for every check, so
> the filter and JAXB always agree on which element they are reading.
> * Refuse with 400 {{MalformedXML}} an element whose prefix is bound to any
> other namespace, and an element name that is not {{prefix:local}}: two
> colons, or an empty prefix or local part ({{:Key}}, {{Key:}}). This parser
> accepts all three; S3 was tested with {{a:b:Key}} only.
> * Keep putting every element in the S3 namespace. A default namespace
> declaration stays uninterpreted, as in S3, which deleted an {{Object}}
> declared in {{urn:foreign}}.
> * Use the same resolved name in the lifecycle element check. That check runs
> before the other checks, so it must receive the resolved name, not the raw
> one.
> * Refuse a DeleteObjects {{Object}} that has no {{Key}} with 400
> {{MalformedXML}}, as S3 does. Check this before the VersionId check and
> before OM is called, so a missing key never reaches the OM client. An empty
> {{Key}} is a different case: S3 answers {{UserKeyMustBeSpecified}} for it,
> and it is not part of this fix.
> * Leave attributes alone. The ACL model binds the raw attribute name
> {{xsi:type}} ({{S3BucketAcl.java:181}}), and that keeps working. A grantee
> type written with another prefix for the XML Schema instance namespace, such
> as {{i:type}}, is not read today either; no client checked writes it (the AWS
> SDKs for Java and the AWS CLI model all use {{xsi}}). It is out of scope here.
> * The ACL part waits for an S3 test of ACL bodies. No ACL body was sent to
> S3, so PutBucketAcl is not changed until S3 has been tested with a prefixed
> {{Grant}} and a prefixed {{AccessControlList}}; then it follows S3. Until
> then PutBucketAcl reads prefixed elements as it does today, so a prefixed
> {{Grant}} still empties the bucket's ACL.
> A {{Key}} with an S3-bound prefix next to a plain {{Key}} is {{Key}} given
> twice, and S3 answers 400 {{MalformedXML}} (tested). After this fix both
> reach JAXB as {{Key}}, so the repeat check of HDDS-17631 refuses the body
> only if it counts resolved names. Counting raw names lets it through
> (measured with a stand-alone model of both checks). A {{Key}} with a
> foreign-bound prefix next to a plain {{Key}} is refused by this fix alone.
> Every check in the filter that looks at element names must use the resolved
> name: the lifecycle check, the repeat check, and the HDDS-17600 check for
> elements inside a value. Otherwise a prefix gets a body past them.
> Not tested against S3: prefixes in lifecycle bodies. Apply the same rule to
> them; if S3 answers differently, follow S3. ACL bodies wait for their own S3
> test, as above.
> Why not the obvious alternatives (both measured):
> * Make the parser namespace-aware. That is the usual way to get local names,
> but it changes more than names. It refuses a prefix that has no declaration,
> which S3 accepts (tested). It also reports {{xsi:type}} as an attribute in
> the XML Schema instance namespace, so the raw-name binding stops matching. On
> {{groupAccessControlList.xml}}, a test resource that follows AWS's
> PutBucketAcl request example, every {{Group}} and {{AmazonCustomerByEmail}}
> grantee is then read as {{CanonicalUser}}.
> * Strip every prefix. That accepts {{<x:Key>}} bound to another namespace,
> which S3 refuses.
> * Refuse elements in another default namespace. S3 accepts them.
> h3. Testing
> * Unit, in the existing test classes, through the S3 endpoints:
> {{TestObjectMultiDelete}}, {{TestMultiDeleteRequestUnmarshaller}},
> {{TestObjectTaggingPut}}, {{TestBucketTaggingPut}},
> {{TestMultipartUploadComplete}}, {{TestBucketAcl}} and
> {{TestPutBucketLifecycleConfigurationUnmarshaller}}. The expected results are
> S3's tested answers where S3 was tested, and the same rule elsewhere. On
> master, 19 of the 20 new tests fail for the reasons above. The other one is a
> control: an {{Object}} in another default namespace is still read. The two
> {{TestBucketAcl}} tests belong to the ACL part and wait with it.
> * Existing tests must keep passing, among them the bodies with no namespace
> or the S3 default namespace, and {{TestBucketAcl}} with both ACL test
> resources.
> * Integration, in {{AbstractS3SDKV1Tests}}: a presigned DeleteObjects request
> with one plain and one prefixed {{Key}}. On master: 500, and both keys are
> kept. After the fix: 200, and both are deleted. A presigned PutBucketAcl
> request with a prefixed {{Grant}} (on master: 200, and the bucket ACL is
> emptied) waits with the ACL part.
> ----
> 🤖 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]
