Skip to content

HDDS-14857. Use XMLUtils.newSecure...Factory#9951

Merged
sarvekshayr merged 4 commits intoapache:masterfrom
Russole:HDDS-14857
Mar 20, 2026
Merged

HDDS-14857. Use XMLUtils.newSecure...Factory#9951
sarvekshayr merged 4 commits intoapache:masterfrom
Russole:HDDS-14857

Conversation

@Russole
Copy link
Copy Markdown
Contributor

@Russole Russole commented Mar 19, 2026

What changes were proposed in this pull request?

  • Replace XML factory instantiation with corresponding newSecure...Factory from XMLUtils.
  • Update TestNodeSchemaLoader to reflect changes in XML parsing behavior

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14857

How was this patch tested?

sarvekshayr
sarvekshayr reviewed Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@sarvekshayr sarvekshayr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for working on this.

I’m concerned about adding hadoop-common as a dependency to hadoop-hdds/config. By pulling in hadoop-common just to use XMLUtils in ConfigFileAppender.java, I see that it provides transitive vulnerable dependency.

Can we look into an alternative?

cc: @adoroszlai

adoroszlai
adoroszlai reviewed Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for the patch.

hadoop-hdds/config/pom.xml
<dependencies>
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m concerned about adding hadoop-common as a dependency to hadoop-hdds/config. By pulling in hadoop-common just to use XMLUtils in ConfigFileAppender.java

Can we look into an alternative?

Thanks @sarvekshayr for raising the concern. Since XMLUtils is annotated as @InterfaceAudience.Private, we may be better off by copying it to Ozone. We can do that as a follow-up, and add exclusions for transitive dependencies now.

Suggested change
<artifactId>hadoop-common</artifactId>
<artifactId>hadoop-common</artifactId>
<exclusions>
<exclusion>
<groupId>*</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>

hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigFileAppender.java Outdated
try {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory();
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FEATURE_SECURE_PROCESSING can be removed.

Suggested change
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);

hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigFileAppender.java Outdated
try {
TransformerFactory factory = TransformerFactory.newInstance();
TransformerFactory factory = XMLUtils.newSecureTransformerFactory();
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FEATURE_SECURE_PROCESSING can be removed.

Suggested change
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/net/NodeSchemaLoader.java Outdated
// Read and parse the schema file.
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
DocumentBuilderFactory dbf = XMLUtils.newSecureDocumentBuilderFactory();
dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);

...p-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/MessageUnmarshaller.java Outdated
context = JAXBContext.newInstance(cls);
saxParserFactory = SAXParserFactory.newInstance();
saxParserFactory = XMLUtils.newSecureSAXParserFactory();
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);

@Russole Russole requested a review from adoroszlai March 20, 2026 11:08
@Russole
Copy link
Copy Markdown
Contributor Author

Russole commented Mar 20, 2026

Thanks @sarvekshayr and @adoroszlai for the review. I’ve updated the patch based on the comments.

@adoroszlai
Copy link
Copy Markdown
Contributor

adoroszlai commented Mar 20, 2026

Thanks @Russole for updating the patch. Sorry, I failed to mention that XMLConstants will become unused.

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/MessageUnmarshaller.java
 31: Unused import - javax.xml.XMLConstants.
hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigFileAppender.java
 25: Unused import - javax.xml.XMLConstants.
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/net/NodeSchemaLoader.java
 31: Unused import - javax.xml.XMLConstants.

@adoroszlai adoroszlai requested a review from sarvekshayr March 20, 2026 12:44
@sarvekshayr sarvekshayr merged commit c5f1dd3 into apache:master Mar 20, 2026
45 checks passed
@sarvekshayr
Copy link
Copy Markdown
Contributor

sarvekshayr commented Mar 20, 2026

Thanks @Russole for the patch and @adoroszlai for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adoroszlai adoroszlai adoroszlai approved these changes

@sarvekshayr sarvekshayr sarvekshayr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Russole @adoroszlai @sarvekshayr