Skip to content

feat: add $HOSTNAME substitution in privilege policy #1360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kevinl03 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: add $HOSTNAME substitution in privilege policy #1360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89d6fe9
feat: add $HOSTNAME substitution in privilege policy
kevinl03 Jan 20, 2026
ed0e6b2
test: add golden file for HOSTNAME substitution test
kevinl03 Jan 20, 2026
fa0864d
feat: add $HOSTNAME substitution in privilege policy
kevinl03 Jan 21, 2026
68813ea
refactor: replace hostname in test files instead of using TestHostnam…
kevinl03 Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions internal/policies/privilege/privilege.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ 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.
// 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
Expand Down Expand Up @@ -175,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)
}
}
Comment on lines +178 to +195
Copy link
Copy Markdown
Member

@denisonbarbosa denisonbarbosa Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I can see the reasoning behind this logic, it's probably unnecessary. Looping through the entries to determine whether we need hostname replacement or not is more costly than just storing the hostname from the get go, so we should just do something like:

hostname, err := os.Hostname()
if err != nil {
    return fmt.Errorf("could not get hostname: %w", err)
}

Makes the code simpler and easier to quickly scan through it


for _, entry := range entries {
var contentSudo string

Expand All @@ -195,8 +214,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, "%") {
Expand Down
72 changes: 72 additions & 0 deletions internal/policies/privilege/privilege_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"context"
"os"
"path/filepath"
"strings"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -34,10 +35,13 @@
"Allow local admins with no other rules is a noop": {entries: []entry.Entry{{Key: "allow-local-admins", Disabled: false}}},

// client admins from AD
"Set client user admins": {entries: []entry.Entry{{Key: "client-admins", Value: "alice@domain.com"}}},

Check failure on line 38 in internal/policies/privilege/privilege_test.go

View workflow job for this annotation

GitHub Actions / Code sanity 

File is not properly formatted (gci)
"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"}}},
"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}}},

Expand Down Expand Up @@ -131,7 +135,75 @@
}
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")
Copy link
Copy Markdown
Member

@denisonbarbosa denisonbarbosa Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are at the end of the tests here, so the messages should be "Teardown: ..." rather than "Setup: ..."

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)

Check failure on line 197 in internal/policies/privilege/privilege_test.go

View workflow job for this annotation

GitHub Actions / Code sanity 

G302: Expect file permissions to be 0600 or less (gosec)
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)
}
}
}
7 changes: 7 additions & 0 deletions ...t_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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-testhost@domain.com"];
});
6 changes: 6 additions & 0 deletions ...den/set_client_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# This file is managed by adsys.
# Do not edit this file manually.
# Any changes will be overwritten.

"secLocalAdmin-testhost@domain.com" ALL=(ALL:ALL) ALL

7 changes: 7 additions & 0 deletions ...ith_multiple_hostname_substitutions/polkit-1/rules.d/00-adsys-privilege-enforcement.rules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"];
});
6 changes: 6 additions & 0 deletions ...ient_admins_with_multiple_hostname_substitutions/sudoers.d/99-adsys-privilege-enforcement
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

7 changes: 7 additions & 0 deletions ...p_admins_with_hostname_substitution/polkit-1/rules.d/00-adsys-privilege-enforcement.rules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"];
});
6 changes: 6 additions & 0 deletions ...t_client_group_admins_with_hostname_substitution/sudoers.d/99-adsys-privilege-enforcement
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Loading