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

executor: remove sensitive information in slow-log and statement #18107

Merged
merged 19 commits into from
Jun 19, 2020
Merged

executor: remove sensitive information in slow-log and statement #18107

merged 19 commits into from
Jun 19, 2020

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Jun 18, 2020 •
edited
Loading

Signed-off-by: crazycs520 [email protected]

What problem does this PR solve?

Before this PR, TiDB slow log will record sensitive information such as passwords in slow-log query.

This PR remove sensitive information in slow-log. Such as below:

execute below sql:

set tidb_slow_log_threshold=0;
create user u1 identified by "456789";
alter user 'u1'@'%' identified by 'abcd';
set password for 'u1'@'%' = 'abcsdafd';

The related slow-log will be:

create user {u1@% password = ***};
alter user {u1@% password = ***};
set password for user u1@%;

the related statement will be:

mysql>select query_sample_text from `STATEMENTS_SUMMARY` where query_sample_text like "set password%" or query_sample_text like "create user%" or query_sample_text like 'alter user%';
+-----------------------------------+
| query_sample_text                 |
+-----------------------------------+
| alter user {u1@% password = ***}  |
| create user {u1@% password = ***} |
| set password for user u1@%        |
+-----------------------------------+

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • Remove sensitive information in slow-log and statement.

@crazycs520 crazycs520 added the security Everything related with security label Jun 18, 2020
@crazycs520 crazycs520 requested a review from a team as a code owner June 18, 2020 05:17
@crazycs520 crazycs520 requested review from qw4990 and removed request for a team June 18, 2020 05:17
@codecov
Copy link

codecov bot commented Jun 18, 2020 •
edited
Loading

Codecov Report

Merging #18107 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18107   +/-   ##
===========================================
  Coverage   80.4869%   80.4869%           
===========================================
  Files           526        526           
  Lines        149377     149377           
===========================================
  Hits         120229     120229           
  Misses        19976      19976           
  Partials       9172       9172

@crazycs520
mask statement too
237a3b9 
Signed-off-by: crazycs520 <[email protected]>
@crazycs520 crazycs520 changed the title executor: remove sensitive information in sow-log executor: remove sensitive information in sow-log and statement. Jun 18, 2020
@crazycs520 crazycs520 changed the title executor: remove sensitive information in sow-log and statement. executor: remove sensitive information in sow-log and statement Jun 18, 2020
@github-actions github-actions bot added the sig/execution SIG execution label Jun 18, 2020
@crazycs520 crazycs520 changed the title executor: remove sensitive information in sow-log and statement executor: remove sensitive information in slow-log and statement Jun 18, 2020
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jun 18, 2020
kennytm
kennytm reviewed Jun 18, 2020
View reviewed changes
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor Author

/run-integration-ddl-test

@crazycs520
Copy link
Contributor Author

/run-unit-tests

@crazycs520
Copy link
Contributor Author

/run-all-tests

@kennytm
Copy link
Contributor

kennytm commented Jun 19, 2020

/run-integration-br-test

@crazycs520
Copy link
Contributor Author

/rebuild

@crazycs520
Copy link
Contributor Author

/run-all-tests

@kennytm
Copy link
Contributor

kennytm commented Jun 19, 2020

/run-common-test
/run-integration-br-test

@crazycs520
Copy link
Contributor Author

/run-integration-br-test

bb7133
bb7133 approved these changes Jun 19, 2020
View reviewed changes
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed sig/sql-infra SIG: SQL Infra labels Jun 19, 2020
@ti-srebot
Copy link
Contributor

Sorry @bb7133, you don't have permission to trigger auto merge event on this branch. You are not a committer for the related sigs:execution(slack).

@crazycs520
Copy link
Contributor Author

/run-integration-br-test

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jun 19, 2020
@crazycs520 crazycs520 merged commit dfca52c into pingcap:master Jun 19, 2020
@crazycs520
Copy link
Contributor Author

/run-cherry-picker

@ti-srebot ti-srebot mentioned this pull request Jun 19, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #18128

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 19, 2020
@crazycs520 @ti-srebot
cherry pick pingcap#18107 to release-3.1
1b1d2ea 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jun 19, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-3.1 in PR #18129

@ti-srebot ti-srebot mentioned this pull request Jun 19, 2020
Merged
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 19, 2020
@crazycs520 @ti-srebot
cherry pick pingcap#18107 to release-4.0
7a1f875 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18130

bb7133 pushed a commit that referenced this pull request Jun 19, 2020
@ti-srebot
executor: remove sensitive information in slow-log and statement (#18107
a285fdf 
) (#18130)
crazycs520 added a commit to ti-srebot/tidb that referenced this pull request Jun 28, 2020
@crazycs520
executor: remove sensitive information in slow-log and statement (pin…
9eb4eeb 
…gcap#18107)

Signed-off-by: crazycs520 <[email protected]>
zz-jason pushed a commit that referenced this pull request Jun 29, 2020
@ti-srebot
executor: remove sensitive information in slow-log ... (#18107) (#18128)
4cbe17a
ti-srebot added a commit that referenced this pull request Jul 6, 2020
@ti-srebot
executor: remove sensitive information in slow-log and statement (#18107
65406be 
) (#18129)

Signed-off-by: ti-srebot <[email protected]>
Signed-off-by: crazycs520 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennytm kennytm kennytm left review comments

@bb7133 bb7133 bb7133 approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
security Everything related with security sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crazycs520 @kennytm @ti-srebot @bb7133