Add disable_group_name_selectors option to Windows workload attestor#6957
Conversation
3da18e8 to
1e9083d
Compare
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you @jananiarunachalam for this contribution!
| |--------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| | ||
| | `discover_workload_path` | If true, the workload path will be discovered by the plugin and used to provide additional selectors | false | | ||
| | `workload_size_limit` | The limit of workload binary sizes when calculating certain selectors (e.g. sha256). If zero, no limit is enforced. If negative, never calculate the hash. | 0 | | ||
| | `disable_group_name_selectors` | If true, skips resolving group SIDs to human-readable names, avoiding expensive Domain Controller lookups. Group SID selectors (`group_sid`) are always collected regardless. Only `group_name` selectors are affected. | false | |
There was a problem hiding this comment.
I wonder if it would help to surface in the Notes section that enabling this flag will cause existing workload registration entries using group_name selectors to stop matching, so operators know to audit those entries (or switch them to group_sid selectors) before flipping it on. As a smaller wording detail, I think that LookupAccount is not strictly a Domain Controller call in every environment (local SAM, AD DS, or other providers can answer); something like "potentially expensive account name resolution (e.g. against a Domain Controller)" might be more accurate.
| processInfo.groups = append(processInfo.groups, enabledSelector+":"+parseAccount(groupAccount, groupDomain)) | ||
| } | ||
| if !disableGroupNames { | ||
| p.log.Debug("lookupAccount (groups) completed", "count", len(groups), "elapsed_ms", time.Since(start).Milliseconds()) |
There was a problem hiding this comment.
A couple of small things on this Debug log. I don't think we have any other elapsed_ms keys already logged. I think telemetry.ElapsedTime and the duration_ms field are the closest that we have, and maybe we could use one of those?
In terms of the wording, maybe "Group account name lookups completed" might feel more familiar.
It could also be worth considering whether this is redundant with the host-level metric from telemetry_workload.StartAttestorCall, which already captures the full per-attestor Attest time and is dominated by this loop for the Windows attestor.
There was a problem hiding this comment.
Addressed all comments.
This debug log specifically isolates the group name resolution portion with a count, which is still useful for diagnosing whether the lookups are the bottleneck.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a configuration option to disable group name selector resolution in the Windows workload attestor to avoid potentially expensive SID-to-name lookups.
Changes:
- Introduces
disable_group_name_selectorsconfiguration flag and plumbs it through attestation. - Skips
LookupAccountfor group SIDs when the flag is enabled, still producinggroup_sidselectors. - Updates docs and adds a test case covering the disabled behavior and expected log message.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/agent/plugin/workloadattestor/windows/windows_windows.go | Adds config flag, conditionally skips group name lookups, logs when enabled |
| pkg/agent/plugin/workloadattestor/windows/windows_windows_test.go | Adds test case for selectors/logs when group name selectors are disabled |
| doc/plugin_agent_workloadattestor_windows.md | Documents the new configuration flag and operational impact |
Comments suppressed due to low confidence (1)
pkg/agent/plugin/workloadattestor/windows/windows_windows.go:1
start := time.Now()is executed even whendisableGroupNamesis true, but the timing is only used when group name lookups are enabled. Move thestartinitialization inside theif !disableGroupNamespath (e.g., set it before running lookups / before the loop only when enabled) to avoid unnecessary work and keep the intent clearer.
//go:build windows
f0b09c0 to
24df7be
Compare
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you @jananiarunachalam!
| p.config = newConfig | ||
|
|
||
| if newConfig.DisableGroupNameSelectors { | ||
| p.log.Info("Group name selectors disabled; skipping LookupAccount for groups, only group_sid selectors will be produced") |
There was a problem hiding this comment.
I'm wondering if this operator-facing message could drop the LookupAccount reference, since that's an internal call name rather than something an operator reading the logs would recognize. Something like "Group name selectors disabled; only group_sid selectors will be produced for groups" might read a little more clearly while still conveying the behavior change.
Signed-off-by: Janani Arunachalam <jarunachala2@bloomberg.net>
Signed-off-by: Janani Arunachalam <jarunachala2@bloomberg.net>
Signed-off-by: Janani Arunachalam <jarunachala2@bloomberg.net>
Signed-off-by: Janani Arunachalam <jarunachala2@bloomberg.net>
Signed-off-by: Janani Arunachalam <jarunachala2@bloomberg.net>
Signed-off-by: Janani Arunachalam <jarunachala2@bloomberg.net>
616b3c4 to
31a4ac0
Compare
Signed-off-by: Janani Arunachalam <jarunachala2@bloomberg.net>
Pull Request check list
Affected functionality
Windows workload attestor group SID resolution.
Description of change
Adds a
disable_group_name_selectorsboolean configuration option (default:false) to the Windows workload attestor. When enabled, the attestor skips resolving group SIDs to human-readable names viaLookupAccountSid, avoiding timeouts for workloads whose principals belong to many groups (~88ms per group, 44+ seconds for 500+ groups). Group SID selectors (group_sid) are always collected regardless of this setting, onlygroup_nameselectors are skipped.Also adds debug-level timing instrumentation to the group name lookup path so operators can observe lookup latency without enabling the flag.
Which issue this PR fixes
fixes #6902