Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Fix-16918][Task] Fix wrong permissions configuration while executing shell #16923

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

[Fix-16918][Task] Fix wrong permissions configuration while executing shell #16923

wants to merge 12 commits into from

Conversation

BruceWong96
Copy link
Contributor

@BruceWong96 BruceWong96 commented Dec 25, 2024 •
edited by SbloodyS
Loading

Purpose of the pull request

close #16918

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)

Pull Request Notice

Pull Request Notice

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

@BruceWong96 BruceWong96 changed the title [Fix-16918][Task] Wrong permissions configuration while executing shell [Fix-16918][Task] Fix wrong permissions configuration while executing shell Dec 25, 2024
ruanwenjun
ruanwenjun reviewed Dec 25, 2024
View reviewed changes
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java Outdated Show resolved Hide resolved
...src/main/java/org/apache/dolphinscheduler/server/worker/utils/TaskExecutionContextUtils.java Outdated
@@ -51,6 +52,9 @@ public static void createTaskInstanceWorkingDirectory(TaskExecutionContext taskE
taskInstanceWorkingDirectory);
}
FileUtils.createDirectoryWith755(Paths.get(taskInstanceWorkingDirectory));
final Path taskInstanceWorkingDirectoryPath = Paths.get(taskInstanceWorkingDirectory);
Copy link
Member

@ruanwenjun ruanwenjun Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we change to 775 and make bootstrap user has the same group with tenant, then tenant will have permission to create file at working directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we change to 775 and make bootstrap user has the same group with tenant, then tenant will have permission to create file at working directory.

So, can i create a function createDirectoryWith775() and call it here ?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can. cc @caishunfeng, @SbloodyS

Copy link
Member

@SbloodyS SbloodyS Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok with this. If we change the task instance working directory to 775, we should add it to the installation docs to tell users add tenant to ds user's group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SbloodyS
Which part of the document should I update?
Also, please review the code i just committed.

Thanks.

Copy link
Member

@SbloodyS SbloodyS Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/zh/guide/security/security.md
https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/guide/security/security.md

The create tenant part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/zh/guide/security/security.md https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/guide/security/security.md

The create tenant part.

Finished.

@SbloodyS SbloodyS added the bug Something isn't working label Dec 26, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Dec 26, 2024
@SbloodyS
Copy link
Member

You should run mvn spotless:apply to format the code. @BruceWong96

@SbloodyS SbloodyS changed the title [Fix-16918][Task] Fix wrong permissions configuration while executing shell [Fix-16918][Task] Fix wrong permissions configuration while executing shell Dec 26, 2024
@BruceWong96
Copy link
Contributor Author

You should run mvn spotless:apply to format the code. @BruceWong96

Finished. Thanks.

@BruceWong96
Copy link
Contributor Author

@caishunfeng @EricGao888 PTAL.

ruanwenjun
ruanwenjun requested changes Dec 27, 2024
View reviewed changes
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java Outdated
} else {
Path parent = path.getParent();
if (parent != null && !parent.toFile().exists()) {
createDirectoryWith755(parent);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
createDirectoryWith755(parent);
createDirectoryWith775(parent);

Can we unify the method createDirectoryWith755 ? these two method has the same logic.
We can create a new method createDirectory(@nonnull Path path, Set permissions);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finished, thanks for your advice. PATL. @ruanwenjun

@github-actions github-actions bot added the test label Jan 7, 2025
SbloodyS
SbloodyS approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

LGTM

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 7, 2025
View reviewed changes
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java
@@ -289,6 +268,29 @@
}
}

public static void createDirectoryWithPermission(@NonNull Path path,
@NonNull Set<PosixFilePermission> permissions) throws IOException {
if (path.toFile().exists()) {

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java
Files.createDirectories(path);
} else {
Path parent = path.getParent();
if (parent != null && !parent.toFile().exists()) {

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java
}

try {
Files.createDirectory(path);

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java
Files.createDirectory(path);
Files.setPosixFilePermissions(path, permissions);
} catch (FileAlreadyExistsException fileAlreadyExistsException) {
log.error("The directory: {} already exists", path);

Check failure

Code scanning / CodeQL

Log Injection High

This log entry depends on a
user-provided value
Loading
.
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jan 7, 2025

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
1 Security Hotspot
64.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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

@SbloodyS SbloodyS SbloodyS approved these changes

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@EricGao888 EricGao888 Awaiting requested review from EricGao888 EricGao888 is a code owner

@ruanwenjun ruanwenjun Awaiting requested review from ruanwenjun ruanwenjun is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@BruceWong96 BruceWong96

Labels
backend bug Something isn't working document test
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

[Bug] [Task] Wrong permissions configuration while executing shell
3 participants
@BruceWong96 @SbloodyS @ruanwenjun