*: add security enhanced mode part 1 #23978

morgo · 2021-04-12T23:47:26Z

What problem does this PR solve?

Problem Summary:

This implements the first of presumably 3-4 PRs to add security enhanced mode. I have a branch available which has almost all of the changes, but it has gotten too large to be reviewed easily.

I will handle the restricted variables / status variables / restricted_user_admin etc in followup PRs.

What is changed and how it works?

Proposal: https://github.com/pingcap/tidb/blob/master/docs/design/2021-03-09-security-enhanced-mode.md

What's Changed:

This introduces some of the basic functionality:

A config setting to enable/disable SEM (with a sysvar that is scoped NONE to see if you are on a server with it enabled).
Hard disabling of SELECT INTO OUTFILE and SHOW CONFIG (security risks)
Modification of the privilege manager to check for an additional role if SEM is enabled on secure tables.
The proposals have also been updated to consistently name dynamic privileges as "RESTRICTED__ADMIN and note that mysql.* privileges should be read only, like information_schema (recently introduced requirement from chatting to @SunRunAway ).

How it Works:

The restrictions are plugged directly into the privilege manager so that there are no risks from certain operations being missed in scope.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test

Side effects

Increased code complexity.
Breaking backward compatibility (when enabled; by default it is backward compatible)

Release note

The 'Security Enhanced Mode' has been introduced. When enabled, it prevents users with the SUPER privilege from performing certain high risk operations. The design is inspired by Security Enhanced (SE) Linux and AppArmor.

morgo · 2021-04-12T23:48:21Z

tidb-server/main.go

+	if cfg.Experimental.EnableEnhancedSecurity {
+		security.Enable()
+	}
+


This is the only real change to this file. The other changes are just cleaning up inconsistencies.

zhouqiang-cl · 2021-04-13T01:50:38Z

/run-all-tests tidb-test=pr/1181

tiancaiamao · 2021-04-13T08:29:11Z

/LGTM

errors.toml

config/config.go

util/security/security.go

morgo · 2021-04-14T01:09:18Z

util/sem/sem.go

+var (
+	semEnabled int32
+)
+
+// Enable enables SEM. This is intended to be used by the test-suite.
+// Dynamic configuration by users may be a security risk.
+func Enable() {
+	atomic.StoreInt32(&semEnabled, 1)
+	variable.SetSysVar(variable.TiDBEnableEnhancedSecurity, variable.BoolOn)
+}
+
+// Disable disables SEM. This is intended to be used by the test-suite.
+// Dynamic configuration by users may be a security risk.
+func Disable() {
+	atomic.StoreInt32(&semEnabled, 0)
+	variable.SetSysVar(variable.TiDBEnableEnhancedSecurity, variable.BoolOff)
+}
+
+// IsEnabled checks if Security Enhanced Mode (SEM) is enabled
+func IsEnabled() bool {
+	return atomic.LoadInt32(&semEnabled) == 1
+}


I changed this to use atomics because I noticed collations used this pattern. Even though there was no race on using the config element, it might have triggered the race detector.

bb7133 · 2021-04-14T14:15:13Z

/LGTM

ti-chi-bot · 2021-04-14T14:15:15Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bb7133
tiancaiamao

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

bb7133 · 2021-04-14T14:15:23Z

Please resolve the conflicts.

…-mode-1

morgo · 2021-04-14T14:49:20Z

/merge

ti-chi-bot · 2021-04-14T14:49:24Z

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

Commit hash: 0552070

morgo · 2021-04-14T15:08:29Z

/run-sqllogic-test-1
/run-unit-test

morgo · 2021-04-14T15:13:04Z

/run-all-tests

bb7133 · 2021-04-14T15:32:51Z

Do we need to merge tidb-test/#1181?

morgo · 2021-04-14T15:40:48Z

Do we need to merge tidb-test/#1181?

No, this is related to a different PR. But yes, it would be great to merge it :-)

bb7133 · 2021-04-14T15:59:03Z

/merge

morgo · 2021-04-14T18:38:07Z

/merge

ti-chi-bot · 2021-04-14T18:38:09Z

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

Commit hash: 7b47277

ti-chi-bot · 2021-04-14T18:38:17Z

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

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.

morgo · 2021-04-15T00:32:40Z

/merge

*: add security enhanced mode part 1

0e441f2

morgo requested review from a team as code owners April 12, 2021 23:47

morgo requested review from XuHuaiyu and removed request for a team April 12, 2021 23:47

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 12, 2021

morgo commented Apr 12, 2021

View reviewed changes

github-actions bot added component/config component/expression sig/sql-infra SIG: SQL Infra labels Apr 12, 2021

morgo requested review from tiancaiamao and bb7133 April 13, 2021 03:50

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 13, 2021

bb7133 reviewed Apr 13, 2021

View reviewed changes

errors.toml Outdated Show resolved Hide resolved

config/config.go Outdated Show resolved Hide resolved

util/security/security.go Outdated Show resolved Hide resolved

add changes for reviewer

fedf4fa

morgo commented Apr 14, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 14, 2021

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2021

Merge remote-tracking branch 'upstream/master' into security-enhanced…

0552070

…-mode-1

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 14, 2021

Merge branch 'master' into security-enhanced-mode-1

1a98b09

empty commit to kick CI

7b47277

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 14, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 14, 2021

Merge branch 'master' into security-enhanced-mode-1

64c5627

ti-chi-bot merged commit 4072172 into pingcap:master Apr 15, 2021

morgo deleted the security-enhanced-mode-1 branch April 16, 2021 01:56

morgo mentioned this pull request Apr 25, 2021

*: add security enhanced mode part 2 #24279

Merged

This was referenced May 4, 2021

*: Add security enhanced mode part 3 #24412

Merged

planner, privilege: Add security enhanced mode part 4 #24416

Merged

morgo mentioned this pull request May 19, 2021

Add documentation for SEM pingcap/docs#5647

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add security enhanced mode part 1 #23978

*: add security enhanced mode part 1 #23978

morgo commented Apr 12, 2021

morgo Apr 12, 2021

zhouqiang-cl commented Apr 13, 2021

tiancaiamao commented Apr 13, 2021

morgo Apr 14, 2021

bb7133 commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

bb7133 commented Apr 14, 2021

morgo commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

morgo commented Apr 14, 2021

morgo commented Apr 14, 2021

bb7133 commented Apr 14, 2021

morgo commented Apr 14, 2021

bb7133 commented Apr 14, 2021

morgo commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

morgo commented Apr 15, 2021

*: add security enhanced mode part 1 #23978

*: add security enhanced mode part 1 #23978

Conversation

morgo commented Apr 12, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

morgo Apr 12, 2021

Choose a reason for hiding this comment

zhouqiang-cl commented Apr 13, 2021

tiancaiamao commented Apr 13, 2021

morgo Apr 14, 2021

Choose a reason for hiding this comment

bb7133 commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

bb7133 commented Apr 14, 2021

morgo commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

morgo commented Apr 14, 2021

morgo commented Apr 14, 2021

bb7133 commented Apr 14, 2021

morgo commented Apr 14, 2021

bb7133 commented Apr 14, 2021

morgo commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

ti-chi-bot commented Apr 14, 2021

morgo commented Apr 15, 2021