diff --git a/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml new file mode 100644 index 00000000000000..a0d7157e3b8bb7 --- /dev/null +++ b/releasenotes/notes/update-programdata-permissions-6271744e48907d71.yaml @@ -0,0 +1,18 @@ +# 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. +upgrade: + - | + 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 ProgramData folder, then you will have to explicitly give the Datadog user + write permissions to the folders and files configured. + 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/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 178782f8892a79..3547d8a8543b50 100644 --- a/test/new-e2e/tests/windows/install-test/installtester.go +++ b/test/new-e2e/tests/windows/install-test/installtester.go @@ -466,6 +466,7 @@ 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, @@ -478,11 +479,27 @@ func (t *Tester) testInstalledFilePermissions(tt *testing.T, ddAgentUserIdentity expected.Access = append(expected.Access, windows.NewExplicitAccessRuleWithFlags( ddAgentUserIdentity, - windows.FileFullControl, + windows.FileReadAndExecute|windows.SYNCHRONIZE, windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, windows.PropagationFlagsNone, ), + windows.NewExplicitAccessRuleWithFlags( + ddAgentUserIdentity, + windows.FileWrite|windows.SYNCHRONIZE, + windows.AccessControlTypeAllow, + 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.PropagationFlagsInherit, + ), ) return expected }, @@ -497,11 +514,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 }, @@ -515,15 +538,53 @@ 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.PropagationFlagsInherit, + ), + windows.NewInheritedAccessRuleWithFlags( + windows.GetIdentityForSID(windows.AdministratorsSID), + windows.FileFullControl, + windows.AccessControlTypeAllow, + windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + windows.PropagationFlagsNone, + ), + 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, + ), + // add creator owner permissions + windows.NewInheritedAccessRuleWithFlags( + windows.GetIdentityForSID("S-1-3-0"), windows.FileFullControl, windows.AccessControlTypeAllow, windows.InheritanceFlagsContainer|windows.InheritanceFlagsObject, + windows.PropagationFlagsInherit, + ), + // create owner inherited permissions + windows.NewInheritedAccessRuleWithFlags( + windows.GetIdentityForSID(windows.LocalSystemSID), + windows.FileFullControl, + windows.AccessControlTypeAllow, windows.PropagationFlagsNone, + windows.InheritanceFlagsNone, ), - ) + } return expected }, }, diff --git a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs index 817f118b0c7970..d3ccf4dd2a9221 100644 --- a/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs +++ b/tools/windows/DatadogAgentInstaller/CustomActions/ConfigureUserCustomActions.cs @@ -446,6 +446,58 @@ 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 + // 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, + InheritanceFlags.None, + PropagationFlags.None, + 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, + 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 @@ -471,6 +523,7 @@ private void ConfigureFilePermissions() if (_ddAgentUserSID != new SecurityIdentifier(WellKnownSidType.LocalSystemSid, null)) { + AddDatadogUserToDataFolder(); GrantAgentAccessPermissions(); } } @@ -579,13 +632,18 @@ public static ActionResult ConfigureUserRollback(Session session) 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"), + 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"), }; } @@ -679,6 +737,8 @@ public ActionResult UninstallUser() _session.Log($"Failed to remove {ddAgentUserName} from {filePath}: {e}"); } } + //remove datadog access to root folder and restore to base permissions + SetBaseInheritablePermissions(); } // We intentionally do NOT delete the ddagentuser account.