From 6c06941d70b057d2b6acfd95dd5529f2fdcbdb9d Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Thu, 12 Dec 2024 18:27:14 -0500 Subject: [PATCH 01/21] Update ProgramFile Directory permissions --- .../ConfigureUserCustomActions.cs | 96 ++++++++++++++++--- 1 file changed, 83 insertions(+), 13 deletions(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 1447f8a080fd03..9a7594af7c5095 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -375,7 +375,7 @@ private bool RemoveRedundantExplicitAccess(string filePath, FileSystemSecurity f private void GrantAgentAccessPermissions() { // add ddagentuser FullControl to select places - foreach (var filePath in PathsWithAgentAccess()) + foreach (var filePath in Chain(PathsWithAgentAccess())) { if (!_fileSystemServices.Exists(filePath)) { @@ -446,6 +446,46 @@ private void GrantAgentAccessPermissions() } } + private void AddDataDogUserToDataFolder(){ + var dataDirectory = _session.Property("APPLICATIONDATADIRECTORY"); + + FileSystemSecurity fileSystemSecurity; + try + { + fileSystemSecurity = _fileSystemServices.GetAccessControl(dataDirectory, AccessControlSections.All); + } + catch (Exception e) + { + _session.Log($"Failed to get ACLs on {dataDirectory}: {e}"); + throw; + } + // ddagentuser Read and execute permissions, enable child inheritance of this ACE + fileSystemSecurity.AddAccessRule(new FileSystemAccessRule( + _ddAgentUserSID, + FileSystemRights.ReadAndExecute | FileSystemRights.Synchronize, + InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, + PropagationFlags.None, + AccessControlType.Allow)); + + // datadog write on this folder + fileSystemSecurity.AddAccessRule(new FileSystemAccessRule( + _ddAgentUserSID, + FileSystemRights.Write, + InheritanceFlags.ContainerInherit, + PropagationFlags.None, + AccessControlType.Allow)); + try + { + UpdateAndLogAccessControl(dataDirectory, fileSystemSecurity); + } + catch (Exception e) + { + _session.Log($"Failed to set ACLs on {dataDirectory}: {e}"); + throw; + } + + } + private void ConfigureFilePermissions() { try @@ -471,6 +511,7 @@ private void ConfigureFilePermissions() if (_ddAgentUserSID != new SecurityIdentifier(WellKnownSidType.LocalSystemSid, null)) { + AddDataDogUserToDataFolder(); GrantAgentAccessPermissions(); } } @@ -577,19 +618,48 @@ public static ActionResult ConfigureUserRollback(Session session) return new ConfigureUserCustomActions(new SessionWrapper(session), "ConfigureUser").ConfigureUserRollback(); } - private List PathsWithAgentAccess() + private List> PathsWithAgentAccess() { - return new List - { - // agent needs to be able to write logs/ - // agent GUI needs to be able to edit config - // agent needs to be able to write to run/ - // agent needs to be able to create auth_token - _session.Property("APPLICATIONDATADIRECTORY"), - // allow agent to write __pycache__ - Path.Combine(_session.Property("PROJECTLOCATION"), "embedded2"), - Path.Combine(_session.Property("PROJECTLOCATION"), "embedded3"), + var configRoot = _session.Property("APPLICATIONDATADIRECTORY"); + var fsEnum = new List>(); + + // directories to process recursively + var dirs = new List { + Path.Combine(configRoot, "conf.d"), + Path.Combine(configRoot, "checks.d"), + Path.Combine(configRoot, "run"), + Path.Combine(configRoot, "logs"), }; + // Add the directories themselves + fsEnum.Add(dirs); + // add their subdirs/files (recursively) + foreach (var dir in dirs) + { + // add dirs only if they exist (EnumerateFileSystemEntries throws an exception if they don't) + if (_fileSystemServices.Exists(dir)) + { + fsEnum.Add(Directory.EnumerateFileSystemEntries(dir, "*.*", SearchOption.AllDirectories)); + } + } + // add specific files + fsEnum.Add(new List + { + Path.Combine(configRoot, "datadog.yaml"), + Path.Combine(configRoot, "system-probe.yaml"), + Path.Combine(configRoot, "auth_token"), + Path.Combine(configRoot, "install_info"), + } + ); + + fsEnum.Add(new List + { + // allow agent to write __pycache__ + Path.Combine(_session.Property("PROJECTLOCATION"), "embedded2"), + Path.Combine(_session.Property("PROJECTLOCATION"), "embedded3"), + } + ); + + return fsEnum; } /// @@ -668,7 +738,7 @@ public ActionResult UninstallUser() if (securityIdentifier != new SecurityIdentifier(WellKnownSidType.LocalSystemSid, null)) { _session.Log($"Removing file access for {ddAgentUserName} ({securityIdentifier})"); - foreach (var filePath in PathsWithAgentAccess()) + foreach (var filePath in Chain(PathsWithAgentAccess())) { try { From 0a02579a393d5009957e142e382f39abcac823ed Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Thu, 12 Dec 2024 18:52:57 -0500 Subject: [PATCH 02/21] fix test permissions --- .../tests/windows/install-test/installtester.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/test/new-e2e/tests/windows/install-test/installtester.go b/test/new-e2e/tests/windows/install-test/installtester.go index e0fbee6886600d..d58dbf89aa0ab9 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -466,24 +466,13 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity path string expectedSecurity func(t *testing.T) windows.ObjectSecurity }{ + //ConfigRoot is only owned by SYSTEM and Administrators { name: "ConfigRoot", path: t.expectedConfigRoot, expectedSecurity: func(tt *testing.T) windows.ObjectSecurity { expected, err := getBaseConfigRootSecurity() require.NoError(tt, err) - if windows.IsIdentityLocalSystem(ddAgentUserIdentity) { - return expected - } - expected.Access = append(expected.Access, - windows.NewExplicitAccessRuleWithFlags( - ddAgentUserIdentity, - windows.FileFullControl, - windows.AccessControlTypeAllow, - windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, - windows.PropagationFlagsNone, - ), - ) return expected }, }, From f67d1fe342029228e325954b76a1505e79363b13 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Thu, 12 Dec 2024 19:15:43 -0500 Subject: [PATCH 03/21] remove recursion of file paths --- .../ConfigureUserCustomActions.cs | 52 +++++-------------- 1 file changed, 13 insertions(+), 39 deletions(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 9a7594af7c5095..a5bff126d089f7 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -375,7 +375,7 @@ private bool RemoveRedundantExplicitAccess(string filePath, FileSystemSecurity f private void GrantAgentAccessPermissions() { // add ddagentuser FullControl to select places - foreach (var filePath in Chain(PathsWithAgentAccess())) + foreach (var filePath in PathsWithAgentAccess()) { if (!_fileSystemServices.Exists(filePath)) { @@ -446,7 +446,7 @@ private void GrantAgentAccessPermissions() } } - private void AddDataDogUserToDataFolder(){ + private void AddDatadogUserToDataFolder(){ var dataDirectory = _session.Property("APPLICATIONDATADIRECTORY"); FileSystemSecurity fileSystemSecurity; @@ -511,7 +511,7 @@ private void ConfigureFilePermissions() if (_ddAgentUserSID != new SecurityIdentifier(WellKnownSidType.LocalSystemSid, null)) { - AddDataDogUserToDataFolder(); + AddDatadogUserToDataFolder(); GrantAgentAccessPermissions(); } } @@ -618,48 +618,22 @@ public static ActionResult ConfigureUserRollback(Session session) return new ConfigureUserCustomActions(new SessionWrapper(session), "ConfigureUser").ConfigureUserRollback(); } - private List> PathsWithAgentAccess() + private List PathsWithAgentAccess() { var configRoot = _session.Property("APPLICATIONDATADIRECTORY"); - var fsEnum = new List>(); - // directories to process recursively - var dirs = new List { + return new List { Path.Combine(configRoot, "conf.d"), Path.Combine(configRoot, "checks.d"), Path.Combine(configRoot, "run"), Path.Combine(configRoot, "logs"), - }; - // Add the directories themselves - fsEnum.Add(dirs); - // add their subdirs/files (recursively) - foreach (var dir in dirs) - { - // add dirs only if they exist (EnumerateFileSystemEntries throws an exception if they don't) - if (_fileSystemServices.Exists(dir)) - { - fsEnum.Add(Directory.EnumerateFileSystemEntries(dir, "*.*", SearchOption.AllDirectories)); - } - } - // add specific files - fsEnum.Add(new List - { - Path.Combine(configRoot, "datadog.yaml"), - Path.Combine(configRoot, "system-probe.yaml"), - Path.Combine(configRoot, "auth_token"), - Path.Combine(configRoot, "install_info"), - } - ); - - fsEnum.Add(new List - { - // allow agent to write __pycache__ - Path.Combine(_session.Property("PROJECTLOCATION"), "embedded2"), - Path.Combine(_session.Property("PROJECTLOCATION"), "embedded3"), - } - ); - - return fsEnum; + Path.Combine(configRoot, "datadog.yaml"), + Path.Combine(configRoot, "system-probe.yaml"), + Path.Combine(configRoot, "auth_token"), + Path.Combine(configRoot, "install_info"), + Path.Combine(_session.Property("PROJECTLOCATION"), "embedded2"), + Path.Combine(_session.Property("PROJECTLOCATION"), "embedded3"), + };; } /// @@ -738,7 +712,7 @@ public ActionResult UninstallUser() if (securityIdentifier != new SecurityIdentifier(WellKnownSidType.LocalSystemSid, null)) { _session.Log($"Removing file access for {ddAgentUserName} ({securityIdentifier})"); - foreach (var filePath in Chain(PathsWithAgentAccess())) + foreach (var filePath in PathsWithAgentAccess()) { try { From 98dafd5f5b5c5880f0c77b6ef46a3c941bc16592 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Thu, 12 Dec 2024 20:37:07 -0500 Subject: [PATCH 04/21] update formating --- .../CustomActions/ConfigureUserCustomActions.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index a5bff126d089f7..7b3479f2beb5df 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -446,7 +446,8 @@ private void GrantAgentAccessPermissions() } } - private void AddDatadogUserToDataFolder(){ + private void AddDatadogUserToDataFolder() + { var dataDirectory = _session.Property("APPLICATIONDATADIRECTORY"); FileSystemSecurity fileSystemSecurity; @@ -466,7 +467,7 @@ private void AddDatadogUserToDataFolder(){ InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, PropagationFlags.None, AccessControlType.Allow)); - + // datadog write on this folder fileSystemSecurity.AddAccessRule(new FileSystemAccessRule( _ddAgentUserSID, @@ -633,7 +634,7 @@ private List PathsWithAgentAccess() Path.Combine(configRoot, "install_info"), Path.Combine(_session.Property("PROJECTLOCATION"), "embedded2"), Path.Combine(_session.Property("PROJECTLOCATION"), "embedded3"), - };; + }; ; } /// From 1306c79d52e7435273897b975cb6d8593f38c58d Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Thu, 12 Dec 2024 22:38:08 -0500 Subject: [PATCH 05/21] fix test permissions --- .../windows/install-test/installtester.go | 41 ++++++++++++++++++- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/test/new-e2e/tests/windows/install-test/installtester.go b/test/new-e2e/tests/windows/install-test/installtester.go index d58dbf89aa0ab9..da974eeeb200b6 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -473,6 +473,22 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity expectedSecurity: func(tt *testing.T) windows.ObjectSecurity { expected, err := getBaseConfigRootSecurity() require.NoError(tt, err) + expected.Access = append(expected.Access, + windows.NewExplicitAccessRuleWithFlags( + ddAgentUserIdentity, + windows.FileReadAndExecute|windows.SYNCHRONIZE, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + windows.PropagationFlagsNone, + ), + windows.NewExplicitAccessRuleWithFlags( + ddAgentUserIdentity, + windows.FileWrite, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer, + windows.PropagationFlagsNone, + ), + ) return expected }, }, @@ -486,11 +502,17 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity return expected } expected.Access = append(expected.Access, - windows.NewInheritedAccessRule( + windows.NewExplicitAccessRule( ddAgentUserIdentity, windows.FileFullControl, windows.AccessControlTypeAllow, ), + // extra inherited rule for ddagentuser + windows.NewInheritedAccessRule( + ddAgentUserIdentity, + windows.FileReadAndExecute|windows.SYNCHRONIZE, + windows.AccessControlTypeAllow, + ), ) return expected }, @@ -505,13 +527,28 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity return expected } expected.Access = append(expected.Access, - windows.NewInheritedAccessRuleWithFlags( + windows.NewExplicitAccessRuleWithFlags( ddAgentUserIdentity, windows.FileFullControl, windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, windows.PropagationFlagsNone, ), + // extra inherited rule for ddagentuser + windows.NewInheritedAccessRuleWithFlags( + ddAgentUserIdentity, + windows.FileReadAndExecute|windows.SYNCHRONIZE, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + windows.PropagationFlagsNone, + ), + windows.NewInheritedAccessRuleWithFlags( + ddAgentUserIdentity, + windows.FileWrite, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer, + windows.PropagationFlagsNone, + ), ) return expected }, From e21c797e2ee727d967c1e4bc5fc91f92c0ea69ef Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Fri, 13 Dec 2024 07:45:05 -0500 Subject: [PATCH 06/21] update localsystem install test --- test/new-e2e/tests/windows/install-test/installtester.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/new-e2e/tests/windows/install-test/installtester.go b/test/new-e2e/tests/windows/install-test/installtester.go index da974eeeb200b6..daa337eeac0b71 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -473,6 +473,9 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity expectedSecurity: func(tt *testing.T) windows.ObjectSecurity { expected, err := getBaseConfigRootSecurity() require.NoError(tt, err) + if windows.IsIdentityLocalSystem(ddAgentUserIdentity) { + return expected + } expected.Access = append(expected.Access, windows.NewExplicitAccessRuleWithFlags( ddAgentUserIdentity, From 544d53c1abb24f0000f36d162988784b4283b7b3 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Fri, 13 Dec 2024 10:35:29 -0500 Subject: [PATCH 07/21] fix windows permissions in install test --- test/new-e2e/tests/windows/install-test/installtester.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/new-e2e/tests/windows/install-test/installtester.go b/test/new-e2e/tests/windows/install-test/installtester.go index daa337eeac0b71..db19209fb0e88b 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -486,7 +486,7 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity ), windows.NewExplicitAccessRuleWithFlags( ddAgentUserIdentity, - windows.FileWrite, + windows.FileWrite|windows.SYNCHRONIZE, windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer, windows.PropagationFlagsNone, @@ -547,7 +547,7 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity ), windows.NewInheritedAccessRuleWithFlags( ddAgentUserIdentity, - windows.FileWrite, + windows.FileWrite|windows.SYNCHRONIZE, windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer, windows.PropagationFlagsNone, From 49d99008122a19a0d621831252d141d22558f2c0 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 16 Dec 2024 15:39:47 -0500 Subject: [PATCH 08/21] Remove DD User access on uninstall --- .../ConfigureUserCustomActions.cs | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 7b3479f2beb5df..2b6443c11a4da1 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -486,6 +486,46 @@ private void AddDatadogUserToDataFolder() } } + + private void RemoveDatadogUserFromDataFolder() + { + var dataDirectory = _session.Property("APPLICATIONDATADIRECTORY"); + + FileSystemSecurity fileSystemSecurity; + try + { + fileSystemSecurity = _fileSystemServices.GetAccessControl(dataDirectory, AccessControlSections.All); + } + catch (Exception e) + { + _session.Log($"Failed to get ACLs on {dataDirectory}: {e}"); + throw; + } + + // Remove ddagentuser from data folder + fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( + _ddAgentUserSID, + FileSystemRights.ReadAndExecute | FileSystemRights.Synchronize, + InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, + PropagationFlags.None, + AccessControlType.Allow)); + fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( + _ddAgentUserSID, + FileSystemRights.Write, + InheritanceFlags.ContainerInherit, + PropagationFlags.None, + AccessControlType.Allow)); + + try + { + UpdateAndLogAccessControl(dataDirectory, fileSystemSecurity); + } + catch (Exception e) + { + _session.Log($"Failed to set ACLs on {dataDirectory}: {e}"); + throw; + } + } private void ConfigureFilePermissions() { @@ -727,6 +767,8 @@ public ActionResult UninstallUser() _session.Log($"Failed to remove {ddAgentUserName} from {filePath}: {e}"); } } + //remove access to root folder + RemoveDatadogUserFromDataFolder(); } // We intentionally do NOT delete the ddagentuser account. From efd698ae3c4b33df70c0cf4c009e95b6ee9d1993 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 16 Dec 2024 15:40:05 -0500 Subject: [PATCH 09/21] release notes --- ...date-programdata-permissions-6271744e48907d71.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml diff --git a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml new file mode 100644 index 00000000000000..91737dc743fb24 --- /dev/null +++ b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml @@ -0,0 +1,11 @@ +# Each section from every release note are combined when the +# CHANGELOG.rst is rendered. So the text needs to be worded so that +# it does not depend on any information only available in another +# section. This may mean repeating some details, but each section +# must be readable independently of the other. +# +# Each section note must be formatted as reStructuredText. +--- +security: + - | + Removes Datadog user's full control of the Datadog data directory on windows. From 45f0b0d5bc30cb161c880e68eeff4cc7cd397d21 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 16 Dec 2024 16:21:05 -0500 Subject: [PATCH 10/21] lint --- .../CustomActions/ConfigureUserCustomActions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 2b6443c11a4da1..857a034341add4 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -486,7 +486,7 @@ private void AddDatadogUserToDataFolder() } } - + private void RemoveDatadogUserFromDataFolder() { var dataDirectory = _session.Property("APPLICATIONDATADIRECTORY"); From 148667f42742569143b04616476a9e3184bdbc85 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 16 Dec 2024 17:55:36 -0500 Subject: [PATCH 11/21] fix argument --- .../CustomActions/ConfigureUserCustomActions.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 857a034341add4..7ec26a9f5c6479 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -487,7 +487,7 @@ private void AddDatadogUserToDataFolder() } - private void RemoveDatadogUserFromDataFolder() + private void RemoveDatadogUserFromDataFolder(SecurityIdentifier sid) { var dataDirectory = _session.Property("APPLICATIONDATADIRECTORY"); @@ -504,13 +504,13 @@ private void RemoveDatadogUserFromDataFolder() // Remove ddagentuser from data folder fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( - _ddAgentUserSID, + sid, FileSystemRights.ReadAndExecute | FileSystemRights.Synchronize, InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, PropagationFlags.None, AccessControlType.Allow)); fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( - _ddAgentUserSID, + sid, FileSystemRights.Write, InheritanceFlags.ContainerInherit, PropagationFlags.None, @@ -768,7 +768,7 @@ public ActionResult UninstallUser() } } //remove access to root folder - RemoveDatadogUserFromDataFolder(); + RemoveDatadogUserFromDataFolder(securityIdentifier); } // We intentionally do NOT delete the ddagentuser account. From 81cb80608c33bf1b628778d5441c6f690dfef851 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Wed, 18 Dec 2024 10:32:26 -0500 Subject: [PATCH 12/21] Update Creator Owner permissions --- .../tests/windows/install-test/assert.go | 3 +- .../windows/install-test/installtester.go | 44 ++++++++++++++++--- .../ConfigureUserCustomActions.cs | 22 ++++++++-- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/test/new-e2e/tests/windows/install-test/assert.go b/test/new-e2e/tests/windows/install-test/assert.go index f0f8087d121aab..3e51db9d634a84 100644 --- a/test/new-e2e/tests/windows/install-test/assert.go +++ b/test/new-e2e/tests/windows/install-test/assert.go @@ -16,9 +16,10 @@ import ( windows "github.com/DataDog/datadog-agent/test/new-e2e/tests/windows/common" windowsAgent "github.com/DataDog/datadog-agent/test/new-e2e/tests/windows/common/agent" + "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "testing" ) // AssertInstalledUserInRegistry checks the registry for the installed user and domain diff --git a/test/new-e2e/tests/windows/install-test/installtester.go b/test/new-e2e/tests/windows/install-test/installtester.go index db19209fb0e88b..4a21ca83e20def 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -488,9 +488,18 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity ddAgentUserIdentity, windows.FileWrite|windows.SYNCHRONIZE, windows.AccessControlTypeAllow, - windows.InheritanceFlagsContainer, + windows.InheritanceFlagsNone, windows.PropagationFlagsNone, ), + // add creator owner permissions + windows.NewExplicitAccessRuleWithFlags( + windows.GetIdentityForSID("S-1-3-0"), + windows.FileFullControl, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + // yes this flag is wrong, but go has the wrong value for it + windows.PropagationFlagsNoPropagate, + ), ) return expected }, @@ -529,7 +538,21 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity if windows.IsIdentityLocalSystem(ddAgentUserIdentity) { return expected } - expected.Access = append(expected.Access, + expected.Access = []windows.AccessRule{ + windows.NewInheritedAccessRuleWithFlags( + windows.GetIdentityForSID(windows.LocalSystemSID), + windows.FileFullControl, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + windows.PropagationFlagsNoPropagate, + ), + windows.NewInheritedAccessRuleWithFlags( + windows.GetIdentityForSID(windows.AdministratorsSID), + windows.FileFullControl, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + windows.PropagationFlagsNone, + ), windows.NewExplicitAccessRuleWithFlags( ddAgentUserIdentity, windows.FileFullControl, @@ -545,14 +568,23 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, windows.PropagationFlagsNone, ), + // add creator owner permissions windows.NewInheritedAccessRuleWithFlags( - ddAgentUserIdentity, - windows.FileWrite|windows.SYNCHRONIZE, + windows.GetIdentityForSID("S-1-3-0"), + windows.FileFullControl, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + windows.PropagationFlagsNoPropagate, + ), + // create owner inherited permissions + windows.NewInheritedAccessRuleWithFlags( + windows.GetIdentityForSID(windows.LocalSystemSID), + windows.FileFullControl, windows.AccessControlTypeAllow, - windows.InheritanceFlagsContainer, windows.PropagationFlagsNone, + windows.InheritanceFlagsNone, ), - ) + } return expected }, }, diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 7ec26a9f5c6479..b8ce0abd4b20af 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -471,10 +471,18 @@ private void AddDatadogUserToDataFolder() // datadog write on this folder fileSystemSecurity.AddAccessRule(new FileSystemAccessRule( _ddAgentUserSID, - FileSystemRights.Write, - InheritanceFlags.ContainerInherit, + FileSystemRights.WriteData | FileSystemRights.AppendData | FileSystemRights.WriteAttributes | FileSystemRights.WriteExtendedAttributes | FileSystemRights.Synchronize, + InheritanceFlags.None, PropagationFlags.None, AccessControlType.Allow)); + + // add full control to CREATOR OWNER + fileSystemSecurity.AddAccessRule(new FileSystemAccessRule( + new SecurityIdentifier(WellKnownSidType.CreatorOwnerSid, null), + FileSystemRights.FullControl, + InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, + PropagationFlags.InheritOnly, + AccessControlType.Allow)); try { UpdateAndLogAccessControl(dataDirectory, fileSystemSecurity); @@ -512,10 +520,18 @@ private void RemoveDatadogUserFromDataFolder(SecurityIdentifier sid) fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( sid, FileSystemRights.Write, - InheritanceFlags.ContainerInherit, + InheritanceFlags.None, PropagationFlags.None, AccessControlType.Allow)); + // remove full control to CREATOR OWNER + fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( + new SecurityIdentifier(WellKnownSidType.CreatorOwnerSid, null), + FileSystemRights.FullControl, + InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, + PropagationFlags.InheritOnly, + AccessControlType.Allow)); + try { UpdateAndLogAccessControl(dataDirectory, fileSystemSecurity); From eb6b7ae3ffd34c40f0ffb22b7c4f8d154bbc655c Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Fri, 20 Dec 2024 12:01:27 -0500 Subject: [PATCH 13/21] add python-cache to agent access list --- .../CustomActions/ConfigureUserCustomActions.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 357a5126591377..df042b7019657f 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -688,6 +688,7 @@ private List PathsWithAgentAccess() Path.Combine(configRoot, "system-probe.yaml"), Path.Combine(configRoot, "auth_token"), Path.Combine(configRoot, "install_info"), + Path.Combine(configRoot, "python-cache"), }; ; } From fdefde26ad498d330705a841e345c2379f695dbf Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Sun, 5 Jan 2025 16:48:09 -0500 Subject: [PATCH 14/21] update for feedback --- test/new-e2e/tests/windows/common/acl.go | 4 +-- .../windows/install-test/installtester.go | 2 +- .../ConfigureUserCustomActions.cs | 27 +++++++++++-------- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/test/new-e2e/tests/windows/common/acl.go b/test/new-e2e/tests/windows/common/acl.go index ddaa5c80f1f037..4a196262bbfec6 100644 --- a/test/new-e2e/tests/windows/common/acl.go +++ b/test/new-e2e/tests/windows/common/acl.go @@ -147,8 +147,8 @@ const ( // https://learn.microsoft.com/en-us/dotnet/api/system.security.accesscontrol.propagationflags const ( PropagationFlagsNone = 0 - PropagationFlagsInherit = 1 - PropagationFlagsNoPropagate = 2 + PropagationFlagsNoPropagate = 1 + PropagationFlagsInherit = 2 ) // Access control types diff --git a/test/new-e2e/tests/windows/install-test/installtester.go b/test/new-e2e/tests/windows/install-test/installtester.go index f28005572956e4..be995fda547b91 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -498,7 +498,7 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, // yes this flag is wrong, but go has the wrong value for it - windows.PropagationFlagsNoPropagate, + windows.PropagationFlagsInherit, ), ) return expected diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index df042b7019657f..df9b4e03b07300 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -469,6 +469,7 @@ private void AddDatadogUserToDataFolder() AccessControlType.Allow)); // datadog write on this folder + // This allows creating new files/folders, but not deleting or modifying permissions. fileSystemSecurity.AddAccessRule(new FileSystemAccessRule( _ddAgentUserSID, FileSystemRights.WriteData | FileSystemRights.AppendData | FileSystemRights.WriteAttributes | FileSystemRights.WriteExtendedAttributes | FileSystemRights.Synchronize, @@ -477,6 +478,8 @@ private void AddDatadogUserToDataFolder() AccessControlType.Allow)); // add full control to CREATOR OWNER + // Grants FullControl to any files/directories created by the Agent user + // Marked InherityOnly so it applies only to children and not this directory fileSystemSecurity.AddAccessRule(new FileSystemAccessRule( new SecurityIdentifier(WellKnownSidType.CreatorOwnerSid, null), FileSystemRights.FullControl, @@ -679,17 +682,19 @@ private List PathsWithAgentAccess() { var configRoot = _session.Property("APPLICATIONDATADIRECTORY"); - return new List { - Path.Combine(configRoot, "conf.d"), - Path.Combine(configRoot, "checks.d"), - Path.Combine(configRoot, "run"), - Path.Combine(configRoot, "logs"), - Path.Combine(configRoot, "datadog.yaml"), - Path.Combine(configRoot, "system-probe.yaml"), - Path.Combine(configRoot, "auth_token"), - Path.Combine(configRoot, "install_info"), - Path.Combine(configRoot, "python-cache"), - }; ; + var filesInConfigRoot = new[] + { + "conf.d", + "checks.d", + "run", + "logs", + "datadog.yaml", + "system-probe.yaml", + "auth_token", + "install_info", + "python-cache", + }.Select(x => Path.Combine(configRoot, x)).ToList(); + return filesInConfigRoot; } /// From 880a3d4f18ed52c2da28b37480ba171a79d89786 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Sun, 5 Jan 2025 17:05:31 -0500 Subject: [PATCH 15/21] update release notes --- .../update-programdata-permissions-6271744e48907d71.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml index 91737dc743fb24..e967f96d7c3f9c 100644 --- a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml +++ b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml @@ -9,3 +9,10 @@ security: - | Removes Datadog user's full control of the Datadog data directory on windows. +upgrade: + - | + The following change removes the Datadog user's full control over the Datadog directory. + If you are using custom configured values for log files, confd_path, run_path, or additional_checksd + that are within the Datadog ProgramDirectory folder, then you will have to explicitly give the Datadog user + write permissions to the folders and files configured. + From 7367dedcf5f8e1fd908a7b17fca341eb145bb001 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Sun, 5 Jan 2025 18:03:27 -0500 Subject: [PATCH 16/21] remove select --- .../ConfigureUserCustomActions.cs | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index df9b4e03b07300..20ec43c0fdc02b 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -681,20 +681,18 @@ public static ActionResult ConfigureUserRollback(Session session) private List PathsWithAgentAccess() { var configRoot = _session.Property("APPLICATIONDATADIRECTORY"); - - var filesInConfigRoot = new[] - { - "conf.d", - "checks.d", - "run", - "logs", - "datadog.yaml", - "system-probe.yaml", - "auth_token", - "install_info", - "python-cache", - }.Select(x => Path.Combine(configRoot, x)).ToList(); - return filesInConfigRoot; + + return new List { + Path.Combine(configRoot, "conf.d"), + Path.Combine(configRoot, "checks.d"), + Path.Combine(configRoot, "run"), + Path.Combine(configRoot, "logs"), + Path.Combine(configRoot, "datadog.yaml"), + Path.Combine(configRoot, "system-probe.yaml"), + Path.Combine(configRoot, "auth_token"), + Path.Combine(configRoot, "install_info"), + Path.Combine(configRoot, "python-cache"), + }; } /// From ec553335634108e3a3af6e0c4e9c812e01d01d71 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Sun, 5 Jan 2025 22:59:03 -0500 Subject: [PATCH 17/21] lint --- .../CustomActions/ConfigureUserCustomActions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 20ec43c0fdc02b..23c4355f47a63a 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -681,7 +681,7 @@ public static ActionResult ConfigureUserRollback(Session session) private List PathsWithAgentAccess() { var configRoot = _session.Property("APPLICATIONDATADIRECTORY"); - + return new List { Path.Combine(configRoot, "conf.d"), Path.Combine(configRoot, "checks.d"), From 80ab32c06ecc5c7079e607fb36b50067d31c47b2 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 6 Jan 2025 11:07:12 -0500 Subject: [PATCH 18/21] update wrong permissions flag --- test/new-e2e/tests/windows/install-test/installtester.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/new-e2e/tests/windows/install-test/installtester.go b/test/new-e2e/tests/windows/install-test/installtester.go index 8ee35beebe6808..3547d8a8543b50 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -544,7 +544,7 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity windows.FileFullControl, windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, - windows.PropagationFlagsNoPropagate, + windows.PropagationFlagsInherit, ), windows.NewInheritedAccessRuleWithFlags( windows.GetIdentityForSID(windows.AdministratorsSID), @@ -574,7 +574,7 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity windows.FileFullControl, windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, - windows.PropagationFlagsNoPropagate, + windows.PropagationFlagsInherit, ), // create owner inherited permissions windows.NewInheritedAccessRuleWithFlags( From 29ac57edb5abf196acb35f0186cba52858756ca6 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 6 Jan 2025 16:38:34 -0500 Subject: [PATCH 19/21] set base to admin/system --- .../ConfigureUserCustomActions.cs | 52 +------------------ 1 file changed, 2 insertions(+), 50 deletions(-) diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 23c4355f47a63a..d3ccf4dd2a9221 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -498,54 +498,6 @@ private void AddDatadogUserToDataFolder() } - private void RemoveDatadogUserFromDataFolder(SecurityIdentifier sid) - { - var dataDirectory = _session.Property("APPLICATIONDATADIRECTORY"); - - FileSystemSecurity fileSystemSecurity; - try - { - fileSystemSecurity = _fileSystemServices.GetAccessControl(dataDirectory, AccessControlSections.All); - } - catch (Exception e) - { - _session.Log($"Failed to get ACLs on {dataDirectory}: {e}"); - throw; - } - - // Remove ddagentuser from data folder - fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( - sid, - FileSystemRights.ReadAndExecute | FileSystemRights.Synchronize, - InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, - PropagationFlags.None, - AccessControlType.Allow)); - fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( - sid, - FileSystemRights.Write, - InheritanceFlags.None, - PropagationFlags.None, - AccessControlType.Allow)); - - // remove full control to CREATOR OWNER - fileSystemSecurity.RemoveAccessRule(new FileSystemAccessRule( - new SecurityIdentifier(WellKnownSidType.CreatorOwnerSid, null), - FileSystemRights.FullControl, - InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, - PropagationFlags.InheritOnly, - AccessControlType.Allow)); - - try - { - UpdateAndLogAccessControl(dataDirectory, fileSystemSecurity); - } - catch (Exception e) - { - _session.Log($"Failed to set ACLs on {dataDirectory}: {e}"); - throw; - } - } - private void ConfigureFilePermissions() { try @@ -785,8 +737,8 @@ public ActionResult UninstallUser() _session.Log($"Failed to remove {ddAgentUserName} from {filePath}: {e}"); } } - //remove access to root folder - RemoveDatadogUserFromDataFolder(securityIdentifier); + //remove datadog access to root folder and restore to base permissions + SetBaseInheritablePermissions(); } // We intentionally do NOT delete the ddagentuser account. From bbd099e9d4e8f4dcf052e93bc8abc56ee2f91c45 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 6 Jan 2025 16:47:56 -0500 Subject: [PATCH 20/21] update release notes --- .../notes/update-programdata-permissions-6271744e48907d71.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml index e967f96d7c3f9c..0d6d48a660f2bd 100644 --- a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml +++ b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml @@ -11,7 +11,7 @@ security: Removes Datadog user's full control of the Datadog data directory on windows. upgrade: - | - The following change removes the Datadog user's full control over the Datadog directory. + Removes Datadog user's full control of the Datadog data directory on windows. If you are using custom configured values for log files, confd_path, run_path, or additional_checksd that are within the Datadog ProgramDirectory folder, then you will have to explicitly give the Datadog user write permissions to the folders and files configured. From 8cd00f3783478c75c60855c4d4efe819bdd0e886 Mon Sep 17 00:00:00 2001 From: Jack Phillips Date: Mon, 6 Jan 2025 17:32:20 -0500 Subject: [PATCH 21/21] release notes --- .../update-programdata-permissions-6271744e48907d71.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml index 0d6d48a660f2bd..a0d7157e3b8bb7 100644 --- a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml +++ b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml @@ -8,11 +8,11 @@ --- security: - | - Removes Datadog user's full control of the Datadog data directory on windows. + Removes Datadog user's full control of the Datadog data directory on Windows. upgrade: - | - Removes Datadog user's full control of the Datadog data directory on windows. + Removes Datadog user's full control of the Datadog data directory on Windows. If you are using custom configured values for log files, confd_path, run_path, or additional_checksd - that are within the Datadog ProgramDirectory folder, then you will have to explicitly give the Datadog user + that are within the Datadog ProgramData folder, then you will have to explicitly give the Datadog user write permissions to the folders and files configured.