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.

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

disable DBG Invoker on default #3831

Conversation

SchrodingerZhu
Copy link
Contributor

Signed-off-by: Schrodinger ZHU Yifan [email protected]

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

disable DBG Invoker on default

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

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

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 10, 2022
@SchrodingerZhu SchrodingerZhu force-pushed the add-way-to-disable-dbg-invoker branch 2 times, most recently from f7e63a3 to b9568dc Compare January 10, 2022 02:08
@SchrodingerZhu
Copy link
Contributor Author

/run-integration-tests

@JaySon-Huang
Copy link
Contributor

Configuration in Settings can be automatically reloaded once the config file is changed. IMO, this is not a "security enough" way to disable DBGInvoker methods.

@SchrodingerZhu
Copy link
Contributor Author

so maybe a more appropriate way is to put the switch to the stable area of configs?

Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
@SchrodingerZhu
Copy link
Contributor Author

/run-integration-tests

@SchrodingerZhu
Copy link
Contributor Author

/run-unit-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jan 11, 2022

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/921/cobertura/
(Coverage detail url is limited office network access)

lines: 43.7% (49840 out of 113995)
branches: 6.4% (81061 out of 1261472)

@JaySon-Huang
Copy link
Contributor

JaySon-Huang commented Jan 11, 2022

"dangerous" does not only mean we can invoke "DBGInvoke" methods. Malicious users can invoke any CK's SQL-like queries statements through its TCP/HTTP port. For example, use "drop table xxx" to drop data, use "insert into" to pollute data.
Only disable invoking "DBGInvoke" methods doesn't help to make TiFlash security enough...

@JaySon-Huang
Copy link
Contributor

Ref #1527

Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
@solotzg
Copy link
Contributor

solotzg commented Jan 11, 2022

We'd better disable all entries towards data manipulation methods.

@JaySon-Huang
Copy link
Contributor

Related issue for tiflash-ctl: JaySon-Huang/tiflash-ctl#7

@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jan 26, 2022

Coverage for changed files

Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/DBGInvoker.cpp               33                32     3.03%          10                 9    10.00%         139                79    43.17%          16                16     0.00%
Interpreters/Context.h             15                10    33.33%          15                10    33.33%          15                10    33.33%           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                              48                42    12.50%          25                19    24.00%         154                89    42.21%          16                16     0.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
16254      9690             40.38%    177898  98249        44.77%

full coverage report (for internal network access only)

@ti-chi-bot
Copy link
Member

@SchrodingerZhu: PR needs rebase.

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 kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants