feat: add $HOSTNAME substitution in privilege policy#1360
feat: add $HOSTNAME substitution in privilege policy#1360kevinl03 wants to merge 4 commits intoubuntu:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1360 +/- ##
==========================================
+ Coverage 91.45% 93.67% +2.21%
==========================================
Files 79 79
Lines 9074 7051 -2023
==========================================
- Hits 8299 6605 -1694
+ Misses 765 436 -329
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
denisonbarbosa
left a comment
denisonbarbosa
left a comment
There was a problem hiding this comment.
Hey, @kevinl03! Thanks for taking the time to contribute to this project. We appreciate it!
Overall, nicely done! However, I think we can improve the hostname substitution part of the PR (well spotted requirement, btw).
| @@ -126,7 +127,20 @@ 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 | |||
| 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) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Adding a public field only for testing to the struct is not recommended. Let's talk about some possible outs for this:
-
You could replace the default
os.Hostnamefunction with a mock one using optional functions, similar to what we do in theinternal/authorizerpackage (look here: 1, 2, 3) -
You could replace the hostname right before updating the golden files in the tests (around here). It's pretty much just replacing the specific hostname with a generic one in the files that were generated.
Number 1 is more elegant but more complex to implement, and since we don't need it for many tests, I'd go with number 2.
There was a problem hiding this comment.
Thanks for the feedback! I've addressed your concerns:
- Removed the
TestHostnamefield from theManagerstruct - Implemented option 2: replace hostname in generated files before golden comparison
- Production code now always uses
os.Hostname()without any test-only fields - All tests pass with machine-independent golden files
The implementation follows the same pattern used elsewhere in the codebase (similar to how integration tests normalize UIDs). Ready for another review when you have a chance.
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: ubuntu#1037
- 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 ubuntu#1037
kevinl03
force-pushed
the
fix-hostname-substitution-1037
branch
from
3cc2a71 to
fa0864d
Compare
…e field Remove TestHostname field from Manager struct. Replace actual hostname with 'testhost' in generated files before golden comparison to ensure machine-independent tests. Fixes ubuntu#1037
| // 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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
|
|
||
| // Get the actual hostname | ||
| hostname, err := os.Hostname() | ||
| require.NoError(t, err, "Setup: Failed to get hostname") |
There was a problem hiding this comment.
We are at the end of the tests here, so the messages should be "Teardown: ..." rather than "Setup: ..."
3 participants
Fixes #1037
Implements $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).
Changes: