Use alternative privilege escalation command to initialize the environment

[darkelf21cn]

What problem does this PR solve?

The su command is used in the EnvInit structure to initialize the tidb service account. However, the command is not always allowed in all the environments. For example, in our Linux environment, sudo is allowed but su and runuser are prohibited. So it's better to determine what is supported first then use the supported one to initialize the environment.

What is changed and how it works?

The PrivilegeEscalationMethod is added to the EnvInit structure which defines all the available privilege escalation methods. When init_env starts, it will first try all the methods until a supported method is found. Then it will use that method to finish the initialization. An error will be raised if there is no privilege escalation method available.

Check List

Tests I'm happy to write unit tests but I didn't write one because the command need to interact with operation system. So I just wrote some code to print the command and run it manually on my test lab. Please advice if you have better suggestions.

package main

import (
	"fmt"
)

// PrivilegeEscalationMethod defiles availabile linux privilege escalation commands
type PrivilegeEscalationMethod int

const (
	// Invalid privilege escalation mathod as a default
	Invalid PrivilegeEscalationMethod = 0
	// Sudo command
	Sudo PrivilegeEscalationMethod = 1
	//Su command
	Su PrivilegeEscalationMethod = 2
	//Runuser command
	Runuser PrivilegeEscalationMethod = 3
)

type EnvInit struct {
	host           string
	deployUser     string
	userGroup      string
	skipCreateUser bool
}

func main() {
	e := &EnvInit{
		host:           "127.0.0.1",
		deployUser:     "tidb",
		userGroup:      "tidb_group",
		skipCreateUser: false,
	}

	// some of the privilege escalation commands might be prohibited by the linux administrator
	// try different methods and choose the one that works (su, runuser, sudo)
	pems := map[PrivilegeEscalationMethod]string{
		Su:      fmt.Sprintf(`su - %s -c 'echo 0'`, e.deployUser),
		Runuser: fmt.Sprintf(`runuser -l %s -c 'echo 0'`, e.deployUser),
		Sudo:    `sudo echo 0`,
	}
	for _, cmd := range pems {
		fmt.Println(cmd)
	}

	fmt.Printf(`su - %s -c 'test -d ~/.ssh || mkdir -p ~/.ssh && chmod 700 ~/.ssh'`+"\n", e.deployUser)
	fmt.Printf(`runuser -l %s -c 'test -d ~/.ssh || mkdir -p ~/.ssh && chmod 700 ~/.ssh'`+"\n", e.deployUser)
	fmt.Printf(`sudo test -d ~/.ssh || sudo mkdir -p ~/.ssh && sudo chmod 700 ~/.ssh && sudo chown %s:%s ~/.ssh`+"\n", e.deployUser, e.userGroup)

	sshAuthorizedKeys := "authorized_keys"
	pk := "AAAAAAA"
	fmt.Printf(`su - %[1]s -c 'grep $(echo %[2]s) %[3]s || echo %[2]s >> %[3]s && chmod 600 %[3]s'`+"\n",
		e.deployUser, pk, sshAuthorizedKeys)
	fmt.Printf(`runuser -l %[1]s -c 'grep $(echo %[2]s) %[3]s || echo %[2]s >> %[3]s && chmod 600 %[3]s'`+"\n",
		e.deployUser, pk, sshAuthorizedKeys)
	fmt.Printf(`sudo grep $(echo %[1]s) %[2]s || sudo echo %[1]s >> %[2]s && chmod 600 %[2]s && sudo chown %[3]s:%[4]s %[2]s`+"\n",
		pk, sshAuthorizedKeys, e.deployUser, e.userGroup)
}

Code changes

• Changes within the function

Side effects

• Increased code complexity

Related changes

None

Release notes:

NONE

[codecov-commenter]

Codecov Report

Merging #799 into master will decrease coverage by 29.59%. The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master     #799       +/-   ##
===========================================
- Coverage   48.41%   18.81%   -29.60%     
===========================================
  Files         262      233       -29     
  Lines       18834    17054     -1780     
===========================================
- Hits         9118     3209     -5909     
- Misses       8270    13373     +5103     
+ Partials     1446      472      -974     

Flag	Coverage Δ
#cluster	?
#dm	?
#integrate	10.73% <ø> (-32.87%)	:arrow_down:
#playground	?
#tiup	10.73% <ø> (ø)
#unittest	18.37% <0.00%> (-0.04%)	:arrow_down:

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

Impacted Files	Coverage Δ
pkg/cluster/task/env_init.go	0.00% <0.00%> (-53.49%)	:arrow_down:
pkg/meta/paths.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/utils/utils.go	0.00% <0.00%> (-100.00%)	:arrow_down:
components/dm/main.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/cluster/prepare.go	0.00% <0.00%> (-100.00%)	:arrow_down:
components/cluster/main.go	0.00% <0.00%> (-100.00%)	:arrow_down:
components/dm/spec/bindversion.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/cluster/template/config/config.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/cluster/template/scripts/scripts.go	0.00% <0.00%> (-100.00%)	:arrow_down:
components/dm/spec/cluster.go	0.00% <0.00%> (-87.50%)	:arrow_down:
... and 177 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 65fdf56...2b53da1. Read the comment docs.

[AstroProfundis]

We have many more locations that uses sudo to get super user privilege, I think we should make this approach more generic. Maybe in executor?

[darkelf21cn]

We have many more locations that uses sudo to get super user privilege, I think we should make this approach more generic. Maybe in executor?

Thank you Allen for reviewing this. I had a quick chat with Long Heng yesterday and he suggested the same thing. There is another issue here that actually we don't need sudo privilege for the tidb service account at all. I noticed that TiUP uses the user account to initialize the TiDB service account and grant the sudo access to it. Then it switches to the service account to continue the deployment. IMHO, it is not necessary because the user account has the full power to do anything on the server. TiUP can just use it to do all the installation stuff then change the file ownership to the service account. In that case, the sudo privilege is not required by the service account at all so the system will be more secure. When we achieve that, this PR will be no longer needed. Long stated he will plan for the change so please feel free to close this one. Cheers~

[AstroProfundis]

Yes the only thing we need from the initial root account is to create the service account (tidb by default) for the deployment, so it's technically possible for us to use that account following on (if it has sudo privilege, or not doing any super-power actions anymore). I believe @lucklove is working on a validation of the idea.

[AstroProfundis]

Hi, is it possible to make the option global (like --native-ssh) and implement this in SSH executor rather than only for the env_init process?

[darkelf21cn]

Hi, is it possible to make the option global (like --native-ssh) and implement this in SSH executor rather than only for the env_init process?

Sure, but I'm quite busy recently. Will take a look at this later.

[ti-chi-bot]

@darkelf21cn: 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.

[codecov-io]

Codecov Report

Merging #799 (12791b3) into master (91466ae) will decrease coverage by 25.53%. The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master     #799       +/-   ##
===========================================
- Coverage   55.41%   29.87%   -25.54%     
===========================================
  Files         279      265       -14     
  Lines       19709    18379     -1330     
===========================================
- Hits        10921     5491     -5430     
- Misses       7070    12039     +4969     
+ Partials     1718      849      -869     

Flag	Coverage Δ
cluster	?
dm	?
integrate	21.52% <ø> (-28.15%)	:arrow_down:
playground	20.29% <ø> (ø)
tiup	16.45% <ø> (ø)
unittest	23.04% <0.00%> (-0.04%)	:arrow_down:

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

Impacted Files	Coverage Δ
pkg/cluster/task/env_init.go	0.00% <0.00%> (-55.89%)	:arrow_down:
pkg/meta/paths.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/utils/utils.go	0.00% <0.00%> (-100.00%)	:arrow_down:
components/dm/main.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/logger/log/log.go	0.00% <0.00%> (-100.00%)	:arrow_down:
components/cluster/main.go	0.00% <0.00%> (-100.00%)	:arrow_down:
components/dm/spec/bindversion.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/cluster/template/config/config.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/cluster/template/scripts/scripts.go	0.00% <0.00%> (-100.00%)	:arrow_down:
pkg/cluster/task/builder.go	0.00% <0.00%> (-97.11%)	:arrow_down:
... and 174 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 91466ae...12791b3. Read the comment docs.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 29.88%. Comparing base (91466ae) to head (12791b3). Report is 690 commits behind head on master.

Files	Patch %	Lines
pkg/cluster/task/env_init.go	0.00%	30 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #799       +/-   ##
===========================================
- Coverage   55.41%   29.88%   -25.53%     
===========================================
  Files         279      265       -14     
  Lines       19709    18379     -1330     
===========================================
- Hits        10921     5491     -5430     
- Misses       7070    12039     +4969     
+ Partials     1718      849      -869     

Flag	Coverage Δ
cluster	?
dm	?
integrate	21.53% <ø> (-28.15%)	:arrow_down:
playground	20.29% <ø> (ø)
tiup	16.46% <ø> (ø)
unittest	23.04% <0.00%> (-0.04%)	:arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Kimi Wang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}