feat: add $HOSTNAME substitution in privilege policy #1360
Conversation
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1360 +/- ##
==========================================
+ Coverage 91.45% 93.68% +2.22%
==========================================
Files 79 79
Lines 9074 7048 -2026
==========================================
- Hits 8299 6603 -1696
+ Misses 765 435 -330
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.
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: