From 89d6fe930ca35d9c5bcd920b4741e700163db8ea Mon Sep 17 00:00:00 2001 From: Kevin Litvin Date: Tue, 20 Jan 2026 15:12:05 -0800 Subject: [PATCH 1/4] feat: add $HOSTNAME substitution in privilege policy Implement $HOSTNAME variable substitution in the Client administrators policy, similar to Windows %COMPUTERNAME% behavior. This allows GPOs to use hostname-specific group names like secLocalAdmin-$HOSTNAME@domain.com. The substitution happens before processing the entry value, affecting both sudoers and polkit configuration files (both old and new formats). Fixes: https://github.com/ubuntu/adsys/issues/1037 --- internal/policies/privilege/privilege.go | 13 +++++++++++-- internal/policies/privilege/privilege_test.go | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/policies/privilege/privilege.go b/internal/policies/privilege/privilege.go index fd7609055..cd8008281 100644 --- a/internal/policies/privilege/privilege.go +++ b/internal/policies/privilege/privilege.go @@ -126,7 +126,13 @@ func (m *Manager) ApplyPolicy(ctx context.Context, objectName string, isComputer log.Debugf(ctx, "Applying privilege policy to %s", objectName) - // We don’t create empty files if there is no entries. Still remove any previous version. + // Get hostname for variable substitution + hostname, err := os.Hostname() + if err != nil { + return fmt.Errorf("can't get hostname: %w", err) + } + + // We don't create empty files if there is no entries. Still remove any previous version. if len(entries) == 0 { if err := os.Remove(sudoersConf); err != nil && !errors.Is(err, fs.ErrNotExist) { return err @@ -195,8 +201,11 @@ func (m *Manager) ApplyPolicy(ctx context.Context, objectName string, isComputer continue } + // Substitute $HOSTNAME with actual hostname + value := strings.ReplaceAll(entry.Value, "$HOSTNAME", hostname) + var polkitElem []string - for _, e := range splitAndNormalizeUsersAndGroups(ctx, entry.Value) { + for _, e := range splitAndNormalizeUsersAndGroups(ctx, value) { contentSudo += fmt.Sprintf("\"%s\" ALL=(ALL:ALL) ALL\n", e) polkitID := fmt.Sprintf("unix-user:%s", e) if strings.HasPrefix(e, "%") { diff --git a/internal/policies/privilege/privilege_test.go b/internal/policies/privilege/privilege_test.go index 3a7da19dc..0c1e26b47 100644 --- a/internal/policies/privilege/privilege_test.go +++ b/internal/policies/privilege/privilege_test.go @@ -38,6 +38,7 @@ func TestApplyPolicy(t *testing.T) { "Set client multiple users admins": {entries: []entry.Entry{{Key: "client-admins", Value: "alice@domain.com,domain\\bob,carole cosmic@otherdomain.com"}}}, "Set client group admins": {entries: []entry.Entry{{Key: "client-admins", Value: "%group@domain.com"}}}, "Set client mixed with users and group admins": {entries: []entry.Entry{{Key: "client-admins", Value: "alice@domain.com,%group@domain.com"}}}, + "Set client admins with HOSTNAME substitution": {entries: []entry.Entry{{Key: "client-admins", Value: "secLocalAdmin-$HOSTNAME@domain.com"}}}, "Empty client AD admins": {entries: []entry.Entry{{Key: "client-admins", Value: ""}}}, "No client AD admins": {entries: []entry.Entry{{Key: "client-admins", Disabled: true}}}, From ed0e6b2f6368760d572f0552cd0a35488a3df23d Mon Sep 17 00:00:00 2001 From: Kevin Litvin Date: Tue, 20 Jan 2026 15:30:43 -0800 Subject: [PATCH 2/4] test: add golden file for HOSTNAME substitution test --- .../polkit-1/rules.d/00-adsys-privilege-enforcement.rules | 7 +++++++ .../sudoers.d/99-adsys-privilege-enforcement | 6 ++++++ 2 files changed, 13 insertions(+) create mode 100644 internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules create mode 100644 internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules new file mode 100644 index 000000000..3383d5645 --- /dev/null +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules @@ -0,0 +1,7 @@ +// This file is managed by adsys. +// Do not edit this file manually. +// Any changes will be overwritten. + +polkit.addAdminRule(function(action, subject){ + return ["unix-user:secLocalAdmin-USERs-MacBook-Pro-4.local@domain.com"]; +}); diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement new file mode 100644 index 000000000..97139fa5c --- /dev/null +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement @@ -0,0 +1,6 @@ +# This file is managed by adsys. +# Do not edit this file manually. +# Any changes will be overwritten. + +"secLocalAdmin-USERs-MacBook-Pro-4.local@domain.com" ALL=(ALL:ALL) ALL + From fa0864d9c88d4816c7761472b3cf928d3b123ab8 Mon Sep 17 00:00:00 2001 From: Kevin Litvin Date: Tue, 20 Jan 2026 23:14:09 -0800 Subject: [PATCH 3/4] feat: add $HOSTNAME substitution in privilege policy - Add TestHostname field to Manager for deterministic testing - Implement $HOSTNAME substitution in client-admins policy entries - Add comprehensive test cases for single, multiple, and group HOSTNAME substitutions - Use normalized test hostname in golden files to ensure machine-independent test outputs Fixes #1037 --- internal/policies/privilege/privilege.go | 14 +++++++++++--- internal/policies/privilege/privilege_test.go | 7 +++++++ .../rules.d/00-adsys-privilege-enforcement.rules | 2 +- .../sudoers.d/99-adsys-privilege-enforcement | 2 +- .../rules.d/00-adsys-privilege-enforcement.rules | 7 +++++++ .../sudoers.d/99-adsys-privilege-enforcement | 6 ++++++ .../rules.d/00-adsys-privilege-enforcement.rules | 7 +++++++ .../sudoers.d/99-adsys-privilege-enforcement | 6 ++++++ 8 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/polkit-1/rules.d/00-adsys-privilege-enforcement.rules create mode 100644 internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/sudoers.d/99-adsys-privilege-enforcement create mode 100644 internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules create mode 100644 internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement diff --git a/internal/policies/privilege/privilege.go b/internal/policies/privilege/privilege.go index cd8008281..ede9bac2c 100644 --- a/internal/policies/privilege/privilege.go +++ b/internal/policies/privilege/privilege.go @@ -86,6 +86,7 @@ type Manager struct { // This is for testing purposes only policyKitSystemDir string + TestHostname string } // NewWithDirs creates a manager with a specific root directory. @@ -127,9 +128,16 @@ func (m *Manager) ApplyPolicy(ctx context.Context, objectName string, isComputer log.Debugf(ctx, "Applying privilege policy to %s", objectName) // Get hostname for variable substitution - hostname, err := os.Hostname() - if err != nil { - return fmt.Errorf("can't get hostname: %w", err) + var hostname string + if m.TestHostname != "" { + // Use test hostname if provided (for testing) + hostname = m.TestHostname + } else { + var hostnameErr error + hostname, hostnameErr = os.Hostname() + if hostnameErr != nil { + return fmt.Errorf("can't get hostname: %w", hostnameErr) + } } // We don't create empty files if there is no entries. Still remove any previous version. diff --git a/internal/policies/privilege/privilege_test.go b/internal/policies/privilege/privilege_test.go index 0c1e26b47..555e4075d 100644 --- a/internal/policies/privilege/privilege_test.go +++ b/internal/policies/privilege/privilege_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/require" @@ -39,6 +40,8 @@ func TestApplyPolicy(t *testing.T) { "Set client group admins": {entries: []entry.Entry{{Key: "client-admins", Value: "%group@domain.com"}}}, "Set client mixed with users and group admins": {entries: []entry.Entry{{Key: "client-admins", Value: "alice@domain.com,%group@domain.com"}}}, "Set client admins with HOSTNAME substitution": {entries: []entry.Entry{{Key: "client-admins", Value: "secLocalAdmin-$HOSTNAME@domain.com"}}}, + "Set client admins with multiple HOSTNAME substitutions": {entries: []entry.Entry{{Key: "client-admins", Value: "x-$HOSTNAME-y-$HOSTNAME@domain.com"}}}, + "Set client group admins with HOSTNAME substitution": {entries: []entry.Entry{{Key: "client-admins", Value: "%admins-$HOSTNAME@domain.com"}}}, "Empty client AD admins": {entries: []entry.Entry{{Key: "client-admins", Value: ""}}}, "No client AD admins": {entries: []entry.Entry{{Key: "client-admins", Disabled: true}}}, @@ -125,6 +128,10 @@ func TestApplyPolicy(t *testing.T) { } m := privilege.NewWithDirs(sudoersDir, policyKitDir, tc.polkitSystemReservedPath) + // Use a fixed test hostname for HOSTNAME substitution tests to ensure golden files are machine-independent + if strings.Contains(name, "HOSTNAME") { + m.TestHostname = "testhost" + } err = m.ApplyPolicy(context.Background(), "ubuntu", !tc.notComputer, tc.entries) if tc.wantErr { require.NotNil(t, err, "ApplyPolicy should have failed but didn't") diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules index 3383d5645..caf0d99bd 100644 --- a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules @@ -3,5 +3,5 @@ // Any changes will be overwritten. polkit.addAdminRule(function(action, subject){ - return ["unix-user:secLocalAdmin-USERs-MacBook-Pro-4.local@domain.com"]; + return ["unix-user:secLocalAdmin-testhost@domain.com"]; }); diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement index 97139fa5c..ef7c9c720 100644 --- a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement @@ -2,5 +2,5 @@ # Do not edit this file manually. # Any changes will be overwritten. -"secLocalAdmin-USERs-MacBook-Pro-4.local@domain.com" ALL=(ALL:ALL) ALL +"secLocalAdmin-testhost@domain.com" ALL=(ALL:ALL) ALL diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/polkit-1/rules.d/00-adsys-privilege-enforcement.rules b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/polkit-1/rules.d/00-adsys-privilege-enforcement.rules new file mode 100644 index 000000000..fe365b6bc --- /dev/null +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/polkit-1/rules.d/00-adsys-privilege-enforcement.rules @@ -0,0 +1,7 @@ +// This file is managed by adsys. +// Do not edit this file manually. +// Any changes will be overwritten. + +polkit.addAdminRule(function(action, subject){ + return ["unix-user:x-testhost-y-testhost@domain.com"]; +}); diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/sudoers.d/99-adsys-privilege-enforcement b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/sudoers.d/99-adsys-privilege-enforcement new file mode 100644 index 000000000..1721ecdd2 --- /dev/null +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_admins_with_multiple_hostname_substitutions/sudoers.d/99-adsys-privilege-enforcement @@ -0,0 +1,6 @@ +# This file is managed by adsys. +# Do not edit this file manually. +# Any changes will be overwritten. + +"x-testhost-y-testhost@domain.com" ALL=(ALL:ALL) ALL + diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules new file mode 100644 index 000000000..ce5b75d2f --- /dev/null +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules @@ -0,0 +1,7 @@ +// This file is managed by adsys. +// Do not edit this file manually. +// Any changes will be overwritten. + +polkit.addAdminRule(function(action, subject){ + return ["unix-group:admins-testhost@domain.com"]; +}); diff --git a/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement new file mode 100644 index 000000000..59e5f8d13 --- /dev/null +++ b/internal/policies/privilege/testdata/TestApplyPolicy/golden/set_client_group_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement @@ -0,0 +1,6 @@ +# This file is managed by adsys. +# Do not edit this file manually. +# Any changes will be overwritten. + +"%admins-testhost@domain.com" ALL=(ALL:ALL) ALL + From 68813ea6022b50b8cae2d33e4640d84478138b21 Mon Sep 17 00:00:00 2001 From: Kevin Litvin Date: Mon, 2 Feb 2026 00:28:11 -0800 Subject: [PATCH 4/4] refactor: replace hostname in test files instead of using TestHostname field Remove TestHostname field from Manager struct. Replace actual hostname with 'testhost' in generated files before golden comparison to ensure machine-independent tests. Fixes #1037 --- internal/policies/privilege/privilege.go | 33 +++++---- internal/policies/privilege/privilege_test.go | 72 +++++++++++++++++-- 2 files changed, 87 insertions(+), 18 deletions(-) diff --git a/internal/policies/privilege/privilege.go b/internal/policies/privilege/privilege.go index ede9bac2c..f5a0b0259 100644 --- a/internal/policies/privilege/privilege.go +++ b/internal/policies/privilege/privilege.go @@ -86,7 +86,6 @@ type Manager struct { // This is for testing purposes only policyKitSystemDir string - TestHostname string } // NewWithDirs creates a manager with a specific root directory. @@ -127,19 +126,6 @@ func (m *Manager) ApplyPolicy(ctx context.Context, objectName string, isComputer log.Debugf(ctx, "Applying privilege policy to %s", objectName) - // Get hostname for variable substitution - var hostname string - if m.TestHostname != "" { - // Use test hostname if provided (for testing) - hostname = m.TestHostname - } else { - var hostnameErr error - hostname, hostnameErr = os.Hostname() - if hostnameErr != nil { - return fmt.Errorf("can't get hostname: %w", hostnameErr) - } - } - // We don't create empty files if there is no entries. Still remove any previous version. if len(entries) == 0 { if err := os.Remove(sudoersConf); err != nil && !errors.Is(err, fs.ErrNotExist) { @@ -189,6 +175,25 @@ func (m *Manager) ApplyPolicy(ctx context.Context, objectName string, isComputer allowLocalAdmins := true var polkitAdditionalUsersGroups []string + // Check if any entry needs hostname substitution + needsHostname := false + for _, entry := range entries { + if entry.Key == "client-admins" && strings.Contains(entry.Value, "$HOSTNAME") { + needsHostname = true + break + } + } + + // Get hostname for variable substitution (only if needed) + var hostname string + if needsHostname { + var hostnameErr error + hostname, hostnameErr = os.Hostname() + if hostnameErr != nil { + return fmt.Errorf("can't get hostname: %w", hostnameErr) + } + } + for _, entry := range entries { var contentSudo string diff --git a/internal/policies/privilege/privilege_test.go b/internal/policies/privilege/privilege_test.go index 555e4075d..f7b3eff7c 100644 --- a/internal/policies/privilege/privilege_test.go +++ b/internal/policies/privilege/privilege_test.go @@ -128,10 +128,6 @@ func TestApplyPolicy(t *testing.T) { } m := privilege.NewWithDirs(sudoersDir, policyKitDir, tc.polkitSystemReservedPath) - // Use a fixed test hostname for HOSTNAME substitution tests to ensure golden files are machine-independent - if strings.Contains(name, "HOSTNAME") { - m.TestHostname = "testhost" - } err = m.ApplyPolicy(context.Background(), "ubuntu", !tc.notComputer, tc.entries) if tc.wantErr { require.NotNil(t, err, "ApplyPolicy should have failed but didn't") @@ -139,7 +135,75 @@ func TestApplyPolicy(t *testing.T) { } require.NoError(t, err, "ApplyPolicy failed but shouldn't have") + // Replace hostname in generated files for tests using $HOSTNAME substitution + // This ensures golden files are machine-independent + needsReplacement := false + for _, entry := range tc.entries { + if entry.Key == "client-admins" && strings.Contains(entry.Value, "$HOSTNAME") { + needsReplacement = true + break + } + } + if needsReplacement { + replaceHostnameInGeneratedFiles(t, tmpRootDir, "testhost") + } + testutils.CompareTreesWithFiltering(t, filepath.Join(tmpRootDir, "etc"), testutils.GoldenPath(t), testutils.UpdateEnabled()) }) } } + +// replaceHostnameInGeneratedFiles replaces the actual hostname with a generic one in generated files +// to ensure golden files are machine-independent. +func replaceHostnameInGeneratedFiles(t *testing.T, rootDir, replacement string) { + t.Helper() + + // Get the actual hostname + hostname, err := os.Hostname() + require.NoError(t, err, "Setup: Failed to get hostname") + if hostname == replacement { + // Already using the replacement, no need to replace + return + } + + // Files that might contain the hostname + files := []string{ + filepath.Join(rootDir, "etc", "sudoers.d", "99-adsys-privilege-enforcement"), + filepath.Join(rootDir, "etc", "polkit-1", "rules.d", "00-adsys-privilege-enforcement.rules"), + filepath.Join(rootDir, "etc", "polkit-1", "localauthority.conf.d", "99-adsys-privilege-enforcement.conf"), + } + + for _, file := range files { + // Check if file exists + if _, err := os.Stat(file); os.IsNotExist(err) { + continue + } + + // Read file content + content, err := os.ReadFile(file) + require.NoError(t, err, "Setup: Failed to read file %s", file) + + // Replace hostname with generic one + newContent := strings.ReplaceAll(string(content), hostname, replacement) + + // Write back if changed + if string(content) != newContent { + // Get original file permissions + info, err := os.Stat(file) + require.NoError(t, err, "Setup: Failed to stat file %s", file) + originalMode := info.Mode() + + // Make file writable temporarily + err = os.Chmod(file, 0644) + require.NoError(t, err, "Setup: Failed to make file writable %s", file) + + // Write the modified content + err = os.WriteFile(file, []byte(newContent), originalMode) + require.NoError(t, err, "Setup: Failed to write file %s", file) + + // Restore original permissions + err = os.Chmod(file, originalMode) + require.NoError(t, err, "Setup: Failed to restore file permissions %s", file) + } + } +}