[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 25.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 40.45%. Comparing base (7c8fa9b) to head (fd251c7).
Report is 1 commits behind head on dev.

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

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

Additional details and impacted files

@@             Coverage Diff              @@
##                dev   #15758      +/-   ##
============================================
- Coverage     40.46%   40.45%   -0.01%     
- Complexity     5195     5196       +1     
============================================
  Files          1378     1379       +1     
  Lines         46084    46093       +9     
  Branches       4923     4924       +1     
============================================
+ Hits          18646    18648       +2     
- Misses        25512    25518       +6     
- Partials       1926     1927       +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

According to docs, there is only List of Strings here.

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

sonarqubecloud · 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. 😢

EricGao888 · 2024-05-14T03:07:15Z

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?

Tests added.

sonarqubecloud · 2024-05-14T03:40:20Z

Quality Gate failed

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

See analysis details on SonarCloud

ruanwenjun

LGTM

an5er

LGTM

qingwli

+1 PTAL @SbloodyS @kezhenxu94

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

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 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

EricGao888 added 4 commits May 13, 2024 22:22

use safe constructor with snake yaml

0bfa7c5

revert changes to HttpTaskDefinitionParser

21e1079

introduce ClassFilterConstructor utility class

82692e7

fix style error

614620d

update

7478774

EricGao888 force-pushed the patch-snake-yaml-load branch from fd621d6 to 7478774 Compare May 13, 2024 14:30

EricGao888 added 3 commits May 14, 2024 10:31

Add classes maybe parsed recursively

f6544fe

Add ut cases to make sure yaml.loadXXX parse file correctly

0847512

update

f606e31

EricGao888 requested review from an5er and kezhenxu94 May 14, 2024 03:07

ruanwenjun approved these changes May 14, 2024

View reviewed changes

ruanwenjun requested review from Radeity and rickchengx and removed request for an5er May 14, 2024 04:40

ruanwenjun added the priority:high label May 14, 2024

ruanwenjun requested a review from an5er May 14, 2024 04:43

an5er reviewed May 14, 2024

View reviewed changes

an5er approved these changes May 14, 2024

View reviewed changes

qingwli approved these changes May 14, 2024

View reviewed changes

ruanwenjun merged commit dc306bf into apache:dev May 14, 2024
61 of 63 checks passed

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

Loading

SbloodyS Mar 22, 2024

EricGao888 Mar 22, 2024

ruanwenjun Mar 23, 2024 •

edited

Loading

an5er Mar 23, 2024

EricGao888 Mar 23, 2024 •

edited

Loading

EricGao888 Mar 23, 2024

codecov-commenter commented Mar 23, 2024 •

edited

Loading

an5er left a comment

kezhenxu94 left a comment

kezhenxu94 Mar 23, 2024

EricGao888 Mar 25, 2024 •

edited

Loading

kezhenxu94 Mar 25, 2024

kezhenxu94 Mar 23, 2024

EricGao888 May 14, 2024

EricGao888 commented Mar 25, 2024

sonarqubecloud bot commented Apr 3, 2024

Liyw979 commented Apr 12, 2024 •

edited

Loading

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 commented May 14, 2024

sonarqubecloud bot commented May 14, 2024

ruanwenjun left a comment

an5er left a comment

qingwli left a comment

[Improvement] Use safe constructor with snake yaml #15758

[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 Loading

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 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricGao888 Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 23, 2024 • edited Loading

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 Loading

Choose a reason for hiding this comment

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

sonarqubecloud bot commented Apr 3, 2024

Quality Gate failed

Liyw979 commented Apr 12, 2024 • edited Loading

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 commented May 14, 2024

sonarqubecloud bot commented May 14, 2024

Quality Gate failed

ruanwenjun left a comment

Choose a reason for hiding this comment

an5er left a comment

Choose a reason for hiding this comment

qingwli left a comment

Choose a reason for hiding this comment

EricGao888 Mar 22, 2024 •

edited

Loading

ruanwenjun Mar 23, 2024 •

edited

Loading

EricGao888 Mar 23, 2024 •

edited

Loading

codecov-commenter commented Mar 23, 2024 •

edited

Loading

EricGao888 Mar 25, 2024 •

edited

Loading

Liyw979 commented Apr 12, 2024 •

edited

Loading