security: disable plugin in default and persist file in specified dir by HuSharp · Pull Request #7087 · tikv/pd

HuSharp · 2023-09-13T09:33:26Z

What problem does this PR solve?

Issue Number: Close #7094

What is changed and how does it work?

Load plugin and persist file in specified dir [config data dir]
Judge persist file name rather than delete this interface to keep compatible
Support a config to decide whether to enable the plugin

Check List

Tests

Unit test

Release note

Add compile configuration to enable `Load Plugin` and disable `Load Plugin` by default. Need to `make pd-server-plugin` when want to enable plugin

ti-chi-bot · 2023-09-13T09:33:28Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lhy1024
nolouch

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-09-13T09:33:28Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: husharp <jinhao.hu@pingcap.com>

codecov · 2023-09-14T09:08:34Z

Codecov Report

Merging #7087 (0859671) into master (e6c8841) will increase coverage by 0.02%.
Report is 2 commits behind head on master.
The diff coverage is 31.81%.

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

@@            Coverage Diff             @@
##           master    #7087      +/-   ##
==========================================
+ Coverage   74.31%   74.33%   +0.02%     
==========================================
  Files         440      440              
  Lines       47217    47202      -15     
==========================================
  Hits        35088    35088              
+ Misses       9034     9015      -19     
- Partials     3095     3099       +4

Flag	Coverage Δ
unittests	`74.33% <31.81%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

lhy1024 · 2023-09-14T09:30:39Z

pkg/schedule/config/config.go

 	defaultEnablePlacementRules  = true
 	defaultEnableWitness         = false
 	defaultHaltScheduling        = false
+	defaultEnableSchedulePlugin  = false


Maybe we need to add a release note about the default value changed

nolouch · 2023-09-18T05:43:40Z

server/api/plugin.go

 // @Failure  500  {string}  string  "PD server failed to proceed the request."
 // @Router   /plugin [post]
 func (h *pluginHandler) LoadPlugin(w http.ResponseWriter, r *http.Request) {
+	if !h.GetScheduleConfig().EnableSchedulePlugin {


I think it's better to be a compile option, and disable by default. otherwise, hackers may enable it.

PTAL! after make pd-server-plugin:

curl -X POST 'http://127.0.0.1:2379/pd/api/v1/plugin' load plugin is disabled, please `make pd-server-plugin` first% curl -X DELETE 'http://127.0.0.1:2379/pd/api/v1/plugin' unload plugin is disabled, please `make pd-server-plugin` first%

cc @lhy1024

how about PLUGIN=1 make pd-server, similiar to SWAGGER and DASHBOARD

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch · 2023-09-22T06:09:25Z

/merge

ti-chi-bot · 2023-09-22T06:09:27Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-09-22T06:09:29Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 0859671

CabinfeverB · 2023-09-22T10:01:46Z

/hold

CabinfeverB · 2023-09-22T10:05:25Z

/hold cancel

ti-chi-bot · 2023-09-22T10:05:43Z

@HuSharp: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

HuSharp · 2023-09-22T10:12:22Z

/hold

HuSharp · 2023-09-22T10:12:30Z

/hold cancel

ti-chi-bot · 2023-09-22T10:29:21Z

In response to a cherrypick label: new pull request created to branch release-6.5: #7141.

ti-chi-bot · 2023-09-22T10:29:59Z

In response to a cherrypick label: new pull request created to branch release-7.1: #7142.

…#7087) (#7141) close #7094 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com>

…#7087) (#7142) close #7094 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Sep 13, 2023

ti-chi-bot bot requested review from nolouch and rleungx September 13, 2023 09:33

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 13, 2023

HuSharp marked this pull request as ready for review September 13, 2023 09:42

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2023

HuSharp force-pushed the check_dir branch from 2b54e2e to a749e00 Compare September 14, 2023 01:48

lhy1024 approved these changes Sep 14, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 14, 2023

HuSharp force-pushed the check_dir branch from a749e00 to ff6cb60 Compare September 14, 2023 08:43

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 14, 2023

HuSharp force-pushed the check_dir branch from ff6cb60 to 22155e0 Compare September 14, 2023 08:47

ti-chi-bot bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Sep 14, 2023

fix

2df55d6

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the check_dir branch from 22155e0 to 2df55d6 Compare September 14, 2023 08:55

HuSharp changed the title ~~api/server:fix data dir to avoid inject virus file~~ security: disable plugin in default and persist file in specified dir Sep 14, 2023

lhy1024 reviewed Sep 14, 2023

View reviewed changes

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 15, 2023

nolouch reviewed Sep 18, 2023

View reviewed changes

config in compile

4df65ba

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the check_dir branch from de6b5de to 4df65ba Compare September 20, 2023 05:16

Merge branch 'master' into check_dir

6c6acb9

lhy1024 approved these changes Sep 21, 2023

View reviewed changes

Merge branch 'master' into check_dir

0859671

nolouch approved these changes Sep 22, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 22, 2023

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 22, 2023

Merge branch 'master' into check_dir

19ee2e0

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2023

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2023

Merge branch 'master' into check_dir

31954ca

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2023

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2023

ti-chi-bot bot merged commit a21fd58 into tikv:master Sep 22, 2023

HuSharp deleted the check_dir branch September 22, 2023 10:28

ti-chi-bot mentioned this pull request Sep 22, 2023

security: disable plugin in default and persist file in specified dir (#7087) #7141

Merged

ti-chi-bot mentioned this pull request Sep 22, 2023

security: disable plugin in default and persist file in specified dir (#7087) #7142

Merged

Conversation

HuSharp commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-chi-bot bot commented Sep 13, 2023

Uh oh!

codecov bot commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lhy1024 Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

HuSharp Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

nolouch Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuSharp Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhy1024 Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

nolouch commented Sep 22, 2023

Uh oh!

ti-chi-bot bot commented Sep 22, 2023

Uh oh!

ti-chi-bot bot commented Sep 22, 2023

Uh oh!

CabinfeverB commented Sep 22, 2023

Uh oh!

CabinfeverB commented Sep 22, 2023

Uh oh!

ti-chi-bot bot commented Sep 22, 2023

Uh oh!

HuSharp commented Sep 22, 2023

Uh oh!

HuSharp commented Sep 22, 2023

Uh oh!

ti-chi-bot commented Sep 22, 2023

Uh oh!

ti-chi-bot commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HuSharp commented Sep 13, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

nolouch Sep 18, 2023 •

edited

Loading

HuSharp Sep 20, 2023 •

edited

Loading