[Improvement] Use safe constructor with snake yaml #15758

EricGao888 · 2024-03-22T14:28:33Z

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

...i/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/AbstractK8sTaskExecutor.java

SbloodyS · 2024-03-22T15:06:15Z

...che/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java

@@ -60,7 +60,7 @@
    }

    protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
-        Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+        Yaml yaml = new Yaml(new SafeConstructor());


Can we fix this? @EricGao888

@SbloodyS In package org.yaml.snakeyaml.constructor;, we could see public class Constructor extends SafeConstructor. Therefore I think there is no need to change it.

I'm not quite sure about it....

I'm not quite sure about it....

I just email an4er and request PR review from him / her to see if there is some extra stuff we need to handle this one.

We can use Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptipon())); to avoid this warning.

https://bitbucket.org/snakeyaml/snakeyaml/src/master/src/test/java/examples/SafeConstructorExampleTest.java

https://bitbucket.org/snakeyaml/snakeyaml/wiki/Documentation#markdown-header-tutorial

First of all I think the code Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class)); can't be changed to Yaml yaml = new Yaml(new SafeConstructor()); .
I would suggest to introduce the class ClassFilterConstructor.java, and then to replace the code in AbstractK8sTaskExecutor to be changed to

import java.util.List; Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] { List.class })).

This is for visibility, because here we expect the user's input to be of type list, you can also remove this map as it is already defined in the SafeConstructor

Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] { }));

I didn't test the security of the parseYamlConfigFile function, so if you want to change it, you can also change to

Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] { LoopTaskYamlDefinition.class }));

Can we fix this? @EricGao888

@SbloodyS Seems I misunderstood your point. Yes, we could fix this warning.

First of all I think the code Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class)); can't be changed to Yaml yaml = new Yaml(new SafeConstructor()); . I would suggest to introduce the class ClassFilterConstructor.java, and then to replace the code in AbstractK8sTaskExecutor to be changed to

import java.util.List; Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] { List.class })).

This is for visibility, because here we expect the user's input to be of type list, you can also remove this map as it is already defined in the SafeConstructor

Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] { }));

I didn't test the security of the parseYamlConfigFile function, so if you want to change it, you can also change to

Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] { LoopTaskYamlDefinition.class }));

@an5er Thanks for your suggestions. It helps a lot.

codecov-commenter · 2024-03-23T15:46:36Z

Codecov Report

Attention: Patch coverage is 30.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 39.10%. Comparing base (0419543) to head (4e2dd67).

❗ Current head 4e2dd67 differs from pull request most recent head fd621d6. Consider uploading reports for the commit fd621d6 to get more accurate results

Files	Patch %	Lines
...scheduler/common/utils/ClassFilterConstructor.java	0.00%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                dev   #15758      +/-   ##
============================================
- Coverage     39.11%   39.10%   -0.02%     
+ Complexity     4887     4886       -1     
============================================
  Files          1326     1327       +1     
  Lines         45206    45213       +7     
  Branches       4818     4819       +1     
============================================
- Hits          17683    17681       -2     
- Misses        25635    25643       +8     
- Partials       1888     1889       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

an5er

LGTM, please ensure that the introduction of security mechanisms does not affect the functionality of the business

kezhenxu94

This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?

kezhenxu94 · 2024-03-23T23:42:37Z

...che/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java

        try (FileReader fileReader = new FileReader(yamlConfigFile)) {
-            return yaml.load(fileReader);
+            return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))


I think, you need to add all nested types(except for the primitive types in Java), such as

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/LoopTaskYamlDefinition.java

Line 29 in 2e16907

private LoopTaskServiceYamlDefinition service;

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/LoopTaskYamlDefinition.java

Line 36 in 2e16907

private LoopTaskAPIYamlDefinition api;

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/LoopTaskYamlDefinition.java

Line 42 in 2e16907

private LoopTaskSubmitMethodYamlDefinition submit;

And so on……

@kezhenxu94 In ClassFilterConstructor, it overrides the method getClassForName from its super class Constructor which is called in the method getClassForNode. The strange thing is that if you put a check point at cl = this.getClassForName(name);, run HttpTaskDefinitionParserTest.parseYamlConfigFile and you will find that cl = this.getClassForName(name); only gets called once, which means the fields and the fields of the fields in LoopTaskYamlDefinition such as LoopTaskServiceYamlDefinition, LoopTaskQueryStateYamlDefinition, etc. are not checked iteratively. I think whether to add these nested types or not does not make any difference and the nested types still bypass the check in this solution.

protected Class<?> getClassForNode(Node node) { Class<? extends Object> classForTag = (Class)this.typeTags.get(node.getTag()); if (classForTag == null) { String name = node.getTag().getClassName(); Class cl; try { cl = this.getClassForName(name); } catch (ClassNotFoundException var6) { throw new YAMLException("Class not found: " + name); } this.typeTags.put(node.getTag(), cl); return cl; } else { return classForTag; } }

I think the method getClassForNode must be called per yaml section node, it is impossible (to me) to get all nested classes types when starting to parse the root node, will check.

kezhenxu94 · 2024-03-23T23:49:26Z

...i/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/AbstractK8sTaskExecutor.java

+        this.yaml = new Yaml(new ClassFilterConstructor(new Class[]{
+                List.class
+        }));


Please also check this, nested non-primitive types should be added too

EricGao888 · 2024-03-25T02:23:58Z

This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?

Sure

sonarcloud · 2024-04-03T03:10:47Z

Quality Gate failed

Failed conditions
21.4% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

Liyw979 · 2024-04-12T02:12:52Z

I am new to DS and have a question here.
Login users are allowed to exectue code on worker side by default, then do we still need to worring about security issues inside modules of org.apache.dolphinscheduler.plugin.task?

an5er · 2024-04-12T07:53:27Z

I am new to DS and have a question here. Login users are allowed to exectue code on worker side by default, then do we still need to worring about security issues inside modules of org.apache.dolphinscheduler.plugin.task?

I think it's normal for the server to execute certain commands to the worker. However, security issues such as those in the org.apache.dolphinscheduler.plugin.task module affect the server, not the worker.

Liyw979 · 2024-04-12T08:20:58Z

Hi @an5er ,do you mean that

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/impl/K8sTaskExecutor.java

Lines 143 to 149 in 08ac132

    
           try { 
        
               if (!StringUtils.isEmpty(commandString)) { 
        
                   commands = yaml.load(commandString.trim()); 
        
               } 
        
               if (!StringUtils.isEmpty(argsString)) { 
        
                   args = yaml.load(argsString.trim()); 
        
               }

runs on the master machine?

an5er · 2024-04-12T08:29:26Z

Hi @an5er ,do you mean that

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/impl/K8sTaskExecutor.java

Lines 143 to 149 in 08ac132

try {

if (!StringUtils.isEmpty(commandString)) {

commands = yaml.load(commandString.trim());

}

if (!StringUtils.isEmpty(argsString)) {

args = yaml.load(argsString.trim());

}

runs on the master machine?

No, it runs on the machine where the dolphinscheduler is deployed, not in the k8s cluster.

caishunfeng · 2024-04-16T08:40:29Z

This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?

Sure

@EricGao888 will you update this pr?

EricGao888 · 2024-04-17T02:56:09Z

This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?

Sure

@EricGao888 will you update this pr?

Yes, but later this week. Quite busy recently. 😢

use safe constructor with snake yaml

a492304

EricGao888 requested a review from kezhenxu94 March 22, 2024 14:28

github-actions bot added the backend label Mar 22, 2024

github-advanced-security bot found potential problems Mar 22, 2024

View reviewed changes

revert changes to HttpTaskDefinitionParser

2cfb2d4

EricGao888 marked this pull request as ready for review March 22, 2024 15:02

EricGao888 requested review from caishunfeng, SbloodyS and zhuangchong as code owners March 22, 2024 15:02

EricGao888 self-assigned this Mar 22, 2024

EricGao888 added the improvement make more easy to user or prompt friendly label Mar 22, 2024

mergeable bot removed the improvement make more easy to user or prompt friendly label Mar 22, 2024

ruanwenjun added the 3.2.2 label Mar 23, 2024

EricGao888 added 2 commits March 23, 2024 23:18

introduce ClassFilterConstructor utility class

0293133

fix style error

d91cfbb

EricGao888 requested review from an5er and ruanwenjun March 23, 2024 15:38

an5er reviewed Mar 23, 2024

View reviewed changes

kezhenxu94 reviewed Mar 23, 2024

View reviewed changes

SbloodyS added the improvement make more easy to user or prompt friendly label Mar 25, 2024

SbloodyS added this to the 3.2.2 milestone Mar 25, 2024

rickchengx and others added 4 commits March 26, 2024 11:41

Merge branch 'dev' into patch-snake-yaml-load

6a7e8a0

Merge branch 'dev' into patch-snake-yaml-load

5277389

Merge branch 'dev' into patch-snake-yaml-load

0545040

Merge branch 'dev' into patch-snake-yaml-load

fd621d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improvement] Use safe constructor with snake yaml #15758

[Improvement] Use safe constructor with snake yaml #15758

EricGao888 commented Mar 22, 2024

SbloodyS Mar 22, 2024

EricGao888 Mar 22, 2024 •

edited

SbloodyS Mar 22, 2024

EricGao888 Mar 22, 2024

ruanwenjun Mar 23, 2024 •

edited

an5er Mar 23, 2024

EricGao888 Mar 23, 2024 •

edited

EricGao888 Mar 23, 2024

codecov-commenter commented Mar 23, 2024 •

edited

an5er left a comment

kezhenxu94 left a comment

kezhenxu94 Mar 23, 2024

EricGao888 Mar 25, 2024 •

edited

kezhenxu94 Mar 25, 2024

kezhenxu94 Mar 23, 2024

EricGao888 commented Mar 25, 2024

sonarcloud bot commented Apr 3, 2024

Liyw979 commented Apr 12, 2024 •

edited

an5er commented Apr 12, 2024

Liyw979 commented Apr 12, 2024

an5er commented Apr 12, 2024

caishunfeng commented Apr 16, 2024

EricGao888 commented Apr 17, 2024

[Improvement] Use safe constructor with snake yaml #15758

Are you sure you want to change the base?

[Improvement] Use safe constructor with snake yaml #15758

Conversation

EricGao888 commented Mar 22, 2024

Purpose of the pull request

Brief change log

Verify this pull request

Choose a reason for hiding this comment

EricGao888 Mar 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanwenjun Mar 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricGao888 Mar 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 23, 2024 • edited

Codecov Report

an5er left a comment

Choose a reason for hiding this comment

kezhenxu94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricGao888 Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricGao888 commented Mar 25, 2024

sonarcloud bot commented Apr 3, 2024

Quality Gate failed

Liyw979 commented Apr 12, 2024 • edited

an5er commented Apr 12, 2024

Liyw979 commented Apr 12, 2024

an5er commented Apr 12, 2024

caishunfeng commented Apr 16, 2024

EricGao888 commented Apr 17, 2024

EricGao888 Mar 22, 2024 •

edited

ruanwenjun Mar 23, 2024 •

edited

EricGao888 Mar 23, 2024 •

edited

codecov-commenter commented Mar 23, 2024 •

edited

EricGao888 Mar 25, 2024 •

edited

Liyw979 commented Apr 12, 2024 •

edited