-
Notifications
You must be signed in to change notification settings - Fork 589
HDDS-7760. Let ContainerDataConstructor extend SafeConstructor #4162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rohit-kb for working on this.
I think we also need to update new Yaml calls in a few places, passing SafeConstructor instance:
ozone/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NodeSchemaLoader.java
Line 230 in a80c6b1
| Yaml yaml = new Yaml(); |
Line 59 in a80c6b1
| Yaml yaml = new Yaml(options); |
Line 74 in a80c6b1
| Yaml yaml = new Yaml(); |
ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestMiniOzoneCluster.java
Line 191 in a80c6b1
| Yaml yaml = new Yaml(); |
|
thanks @adoroszlai, just want to know a little about the role of SafeConstructor instance in Yaml object creations. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rohit-kb for updating the patch.
| options.setPrettyFlow(true); | ||
| options.setDefaultFlowStyle(DumperOptions.FlowStyle.FLOW); | ||
| Yaml yaml = new Yaml(options); | ||
| Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions()), new Representer(options), options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/DatanodeIdYaml.java
62: Line is longer than 80 characters (found 102).
|
|
||
| try { | ||
| Yaml yaml = new Yaml(); | ||
| Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this causes test failures:
Tests run: 5, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0.268 s <<< FAILURE! - in org.apache.hadoop.hdds.scm.net.TestYamlSchemaLoader
Can you please check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review, looking into it
This reverts commit cacbccd.
What changes were proposed in this pull request?
Finding and replacing Constructor() class of snakeyaml with SafeConstructor() due to CVE-2022-1471
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7760