Add new config param to allow GroupExec perm for the SecretBackend command #6298

clamoriniere · 2020-08-28T15:55:15Z

What does this PR do?

To allow the execution of the secret-backend command packaged inside
the docker image. the secret-backend script need to allow the execution
of a group uid.
The new option "secret_backend_command_allow_group_exec_perm" allow to
change the behaviour of the checkRights() function and allow GroupExecPerm.

We update only the dockerfile configuration for the cluster-agent. because
the "agent" process needs to run as root, so the described setup is not possible
with the agent image:

change the readsecret.sh permission to 510
set DD_SECRET_BACKEND_COMMAND_ALLOW_GROUP_EXEC_PERM env var to true
by default in the cluster-agent entry-point

Motivation

When the agent runs in a container environment, the process can be
executed with a "random" uid (for security reason)

Additional Notes

Anything else we should know when reviewing?

Describe your test plan

Start the Cluster-Agent and use the "secret-backend" script packaged in the image readSecret.py. The Cluster-Agent should not return a file permission check error.

Dockerfiles/cluster-agent/amd64/Dockerfile

L3n41c

The biggest issue is the lack of read permission for a script.

Otherwise, I left some cosmetics suggestions.

Dockerfiles/cluster-agent/arm64/Dockerfile

pkg/secrets/check_rights_nix.go

pkg/secrets/check_rights_nix_test.go

pkg/secrets/no_secrets.go

pkg/secrets/secrets.go

L3n41c

There is only one error message which is misleading.

pkg/secrets/check_rights_nix.go

hush-hush

A few comments. But overall if security is OK with this I think we're almost good to merge.

pkg/secrets/check_rights_nix.go

pkg/secrets/check_rights.go

pkg/secrets/check_rights_windows.go

releasenotes/notes/Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fad50b.yaml

pkg/secrets/secrets.go

hush-hush · 2020-09-03T18:16:17Z

Once this is merged let's not forget to update the official doc to explain when, why and how to enable this option : https://docs.datadoghq.com/agent/guide/secrets-management/?tab=linux

pkg/secrets/check_rights_nix.go

olivielpeau

Left a few nits inline, once addressed LGTM 👍

Let's have a security review as well

pkg/secrets/check_rights_nix.go

pkg/secrets/secrets.go

releasenotes/notes/Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fad50b.yaml

pkg/secrets/check_rights_nix.go

pkg/secrets/check_rights_windows.go

pkg/secrets/check_rights_nix_test.go

pkg/secrets/check_rights_nix.go

hush-hush

I think it's almost good to go for me.

releasenotes/notes/Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fad50b.yaml

releasenotes-dca/Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fad50b.yaml

pkg/secrets/check_rights_nix_test.go

hush-hush

One nit but looks good otherwise

pkg/secrets/check_rights_nix.go

olivielpeau

Nits, and one comment on the logged warning, but the logic LGTM! 👍 (and thanks for the additional unit tests!)

olivielpeau · 2020-09-17T08:41:30Z

pkg/secrets/check_rights_nix.go

 	}

-	// checking that we own the executable
-	usr, err := user.Current()
+	userGroups, err := usr.GroupIds()
 	if err != nil {
 		return fmt.Errorf("can't query current user UID: %s", err)


Suggested change

return fmt.Errorf("can't query current user UID: %s", err)

return fmt.Errorf("can't query current user's GIDs: %s", err)

pkg/secrets/check_rights_nix.go

pkg/secrets/secrets.go

releasenotes-dca/Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fad50b.yaml

pkg/secrets/check_rights_nix_test.go

trishankatdatadog

Other than warning that a script is executable but not readable (very minor comment), and adding tests for checking UID, I see no security issue. Thanks for all your work!

trishankatdatadog · 2020-09-16T21:00:41Z

pkg/secrets/check_rights_nix.go

+	return nil
+}
+
+// checkUserPermission check that only the current User or one of his group can exec the path


Suggested change

// checkUserPermission check that only the current User or one of his group can exec the path

// checkGroupPermission check that only the current User or one of his group can exec the path

trishankatdatadog · 2020-09-16T21:02:34Z

pkg/secrets/check_rights_windows.go

@@ -24,7 +24,9 @@ var (

 // checkRights check that the given filename has access controls set only for
 // Administrator, Local System and the datadog user.
-func checkRights(filename string) error {
+func checkRights(filename string, _ bool) error {


Maybe instead of ignoring the bool here, return an error if it is true?

trishankatdatadog · 2020-09-17T15:51:04Z

pkg/secrets/check_rights_nix.go

+		isUserFile = true
+	}
+	// If the file is not own by the user, lets check for on of his groups
+	if !isUserFile {


if isUserFile, you might wish to check that user can exec (check that stat.Mode&syscall.S_IXUSR == 0 is not an issue like in checkUserPermission)

trishankatdatadog · 2020-09-17T16:16:03Z

pkg/secrets/check_rights_nix_test.go

 	require.Nil(t, os.Chmod(tmpfile.Name(), 0600))
-	require.NotNil(t, checkRights(tmpfile.Name()))
+	require.NotNil(t, checkRights(tmpfile.Name(), allowGroupExec))

 	// group should have no right
 	require.Nil(t, os.Chmod(tmpfile.Name(), 0710))


Consider adding a separate 0720 test here (group can write)

trishankatdatadog · 2020-09-17T16:16:22Z

pkg/secrets/check_rights_nix_test.go


 	// group should have no right
 	require.Nil(t, os.Chmod(tmpfile.Name(), 0710))
-	require.NotNil(t, checkRights(tmpfile.Name()))
+	require.NotNil(t, checkRights(tmpfile.Name(), allowGroupExec))

 	// other should have no right
 	require.Nil(t, os.Chmod(tmpfile.Name(), 0701))


Consider adding a separate 0702 test here (others can write)

trishankatdatadog · 2020-09-17T16:17:06Z

pkg/secrets/check_rights_nix_test.go

+	require.Nil(t, os.Chmod(tmpfile.Name(), 0770))
+	require.NotNil(t, checkRights(tmpfile.Name(), allowGroupExec))
+
+	// other should have no right


Consider adding a separate 0702 test here (others can write)

When the agent runs in a container environment, the process can be executed with a "random" uid (for security reason). To allow the execution of the secret-backend command packaged inside the docker image. the secret-backend script need to allow the execution of a group uid. The new option "secret_backend_command_allow_group_exec_perm" allow to change the behaviour of the `checkRights()` function and allow GroupExecPerm. We update only the dockerfile configuration for the cluster-agent. because the "agent" process needs to run as root, so the described setup is not possible with the agent image: * change the readsecret.sh permission to 510 * set DD_SECRET_BACKEND_COMMAND_ALLOW_GROUP_EXEC_PERM env var to true by default in the cluster-agent entry-point

Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com>

Co-authored-by: Olivier Vielpeau <olivielpeau@users.noreply.github.com>

…d50b.yaml

Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

Co-authored-by: maxime mouial <hush-hush@users.noreply.github.com>

Co-authored-by: Olivier Vielpeau <olivielpeau@users.noreply.github.com>

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

clamoriniere added the component/config label Aug 28, 2020

clamoriniere added this to the 7.23.0 milestone Aug 28, 2020

clamoriniere requested review from a team as code owners August 28, 2020 15:55

clamoriniere added the team/containers label Aug 28, 2020

clamoriniere force-pushed the clamoriniere/fix-checks-permission-v1 branch from 5f889b5 to 850895d Compare August 31, 2020 07:30

clamoriniere changed the title ~~Allow GroupExec perm for the SecretBackend command~~ Add new config param to allow GroupExec perm for the SecretBackend command Aug 31, 2020

L3n41c requested changes Aug 31, 2020

View reviewed changes

Dockerfiles/cluster-agent/amd64/Dockerfile Outdated Show resolved Hide resolved

L3n41c requested changes Aug 31, 2020

View reviewed changes

clamoriniere force-pushed the clamoriniere/fix-checks-permission-v1 branch 2 times, most recently from 855c4c7 to fee5f73 Compare August 31, 2020 11:44

clamoriniere requested a review from L3n41c August 31, 2020 11:44

L3n41c approved these changes Aug 31, 2020

View reviewed changes

pkg/secrets/check_rights_nix.go Outdated Show resolved Hide resolved

olivielpeau requested a review from hush-hush September 2, 2020 11:22

hush-hush requested changes Sep 3, 2020

View reviewed changes

hush-hush requested a review from a team September 3, 2020 18:16

hush-hush requested changes Sep 3, 2020

View reviewed changes

pkg/secrets/check_rights_nix.go Outdated Show resolved Hide resolved

pkg/secrets/check_rights_nix.go Outdated Show resolved Hide resolved

olivielpeau requested review from trishankatdatadog and removed request for a team September 8, 2020 18:15

olivielpeau approved these changes Sep 8, 2020

View reviewed changes

trishankatdatadog reviewed Sep 8, 2020

View reviewed changes

pkg/secrets/check_rights_nix.go Outdated Show resolved Hide resolved

trishankatdatadog reviewed Sep 8, 2020

View reviewed changes

pkg/secrets/check_rights_nix.go Outdated Show resolved Hide resolved

clamoriniere force-pushed the clamoriniere/fix-checks-permission-v1 branch from 79fe747 to 44fcf25 Compare September 8, 2020 22:47

trishankatdatadog reviewed Sep 9, 2020

View reviewed changes

pkg/secrets/check_rights_windows.go Outdated Show resolved Hide resolved

pkg/secrets/check_rights_nix_test.go Outdated Show resolved Hide resolved

pkg/secrets/check_rights_nix_test.go Show resolved Hide resolved

pkg/secrets/check_rights_nix.go Outdated Show resolved Hide resolved

hush-hush requested changes Sep 14, 2020

View reviewed changes

releasenotes/notes/Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fad50b.yaml Show resolved Hide resolved

releasenotes-dca/Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fad50b.yaml Outdated Show resolved Hide resolved

clamoriniere commented Sep 15, 2020

View reviewed changes

pkg/secrets/check_rights_nix_test.go Show resolved Hide resolved

hush-hush approved these changes Sep 16, 2020

View reviewed changes

clamoriniere force-pushed the clamoriniere/fix-checks-permission-v1 branch from 10af196 to 9550d63 Compare September 16, 2020 18:58

hush-hush approved these changes Sep 17, 2020

View reviewed changes

pkg/secrets/check_rights_nix.go Outdated Show resolved Hide resolved

olivielpeau approved these changes Sep 17, 2020

View reviewed changes

trishankatdatadog approved these changes Sep 17, 2020

View reviewed changes

clamoriniere and others added 16 commits September 18, 2020 13:27

Update pkg/secrets/check_rights_nix.go

cdb406a

Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com>

Refactor and update code after reviews

73539fd

Apply suggestions from code review

d6f9326

Co-authored-by: Olivier Vielpeau <olivielpeau@users.noreply.github.com>

fix checkRights() func after review

7089f32

update releasenote for agent and DCA

5f0ea08

add log information about secret backend cmd perms

56279a4

Update check_rights_windows.go

6d79f06

Update Add-Group-exec-perm-for-the-SecretBackend-command-a7fbe1ece1fa…

4411d4e

…d50b.yaml

Apply suggestions from code review

9ec6db3

Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>

Update check_rights_nix_test.go

e3f5a10

new checkRights implementation

85474aa

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

add new unit-test

6e6cedc

Update pkg/secrets/check_rights_nix.go

18015a2

Co-authored-by: maxime mouial <hush-hush@users.noreply.github.com>

Apply suggestions from code review

548c5af

Co-authored-by: Olivier Vielpeau <olivielpeau@users.noreply.github.com>

Add more unit-test and comment fixes

1c431ca

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>

clamoriniere force-pushed the clamoriniere/fix-checks-permission-v1 branch from f587136 to 1c431ca Compare September 18, 2020 11:28

clamoriniere merged commit b05aa49 into master Sep 18, 2020

clamoriniere deleted the clamoriniere/fix-checks-permission-v1 branch September 18, 2020 13:13

ahmed-mez mentioned this pull request Sep 24, 2020

[DCA] Fix cluster agent commands #6445

Merged

celenechang mentioned this pull request Mar 10, 2022

Update permission of secret backend scripts for cluster check runners #11246

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new config param to allow GroupExec perm for the SecretBackend command #6298

Add new config param to allow GroupExec perm for the SecretBackend command #6298

clamoriniere commented Aug 28, 2020 •

edited

Loading

L3n41c left a comment

L3n41c left a comment

hush-hush left a comment

hush-hush commented Sep 3, 2020

olivielpeau left a comment

hush-hush left a comment

hush-hush left a comment

olivielpeau left a comment

olivielpeau Sep 17, 2020

trishankatdatadog left a comment

trishankatdatadog Sep 16, 2020

trishankatdatadog Sep 16, 2020

trishankatdatadog Sep 17, 2020

trishankatdatadog Sep 17, 2020

trishankatdatadog Sep 17, 2020

trishankatdatadog Sep 17, 2020

	return fmt.Errorf("can't query current user UID: %s", err)
	return fmt.Errorf("can't query current user's GIDs: %s", err)

	// checkUserPermission check that only the current User or one of his group can exec the path
	// checkGroupPermission check that only the current User or one of his group can exec the path

Add new config param to allow GroupExec perm for the SecretBackend command #6298

Add new config param to allow GroupExec perm for the SecretBackend command #6298

Conversation

clamoriniere commented Aug 28, 2020 • edited Loading

What does this PR do?

Motivation

Additional Notes

Describe your test plan

L3n41c left a comment

Choose a reason for hiding this comment

L3n41c left a comment

Choose a reason for hiding this comment

hush-hush left a comment

Choose a reason for hiding this comment

hush-hush commented Sep 3, 2020

olivielpeau left a comment

Choose a reason for hiding this comment

hush-hush left a comment

Choose a reason for hiding this comment

hush-hush left a comment

Choose a reason for hiding this comment

olivielpeau left a comment

Choose a reason for hiding this comment

olivielpeau Sep 17, 2020

Choose a reason for hiding this comment

trishankatdatadog left a comment

Choose a reason for hiding this comment

trishankatdatadog Sep 16, 2020

Choose a reason for hiding this comment

trishankatdatadog Sep 16, 2020

Choose a reason for hiding this comment

trishankatdatadog Sep 17, 2020

Choose a reason for hiding this comment

trishankatdatadog Sep 17, 2020

Choose a reason for hiding this comment

trishankatdatadog Sep 17, 2020

Choose a reason for hiding this comment

trishankatdatadog Sep 17, 2020

Choose a reason for hiding this comment

clamoriniere commented Aug 28, 2020 •

edited

Loading