spire-agent: try to enable SE_DEBUG_PRIVILEGE at startup on windows#7073
spire-agent: try to enable SE_DEBUG_PRIVILEGE at startup on windows#7073sorindumitru wants to merge 2 commits into
Conversation
The [OpenProcessToken](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocesstoken) API mentions that SE_DEBUG_NAME/SeDebugPrivilege (why two different names, windows???) privilege is required for opening the process token for a process running as a more privileged account (it does work from admin to admin users without this). Attempt to enable this and log an warning if that fails. Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
sorindumitru
requested review from
MarcosDY,
amartinezfayo,
evan2645 and
rturner3
as code owners
There was a problem hiding this comment.
Pull request overview
This PR updates the SPIRE Agent Windows startup path to attempt enabling SeDebugPrivilege so the Windows workload attestor can query tokens for processes running under more privileged accounts, and documents the operational requirement.
Changes:
- Attempt to enable
SeDebugPrivilegeduringspire-agent runstartup on Windows (best-effort; warn on failure). - Document the need for
SE_DEBUG_NAMEwhen attesting higher-privileged workloads on Windows.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| doc/plugin_agent_workloadattestor_windows.md | Documents SE_DEBUG_NAME requirement and startup behavior for higher-privileged workload attestation. |
| cmd/spire-agent/cli/run/run_windows.go | Adds Windows startup logic to enable SeDebugPrivilege via token privilege adjustment. |
|
|
||
| return windows.AdjustTokenPrivileges(token, false, &tp, 0, nil, nil) |
There was a problem hiding this comment.
This is not wrong, but I wonder how safe this is. GetLastError seems to return a per thread error and I'm not sure how that maps to goroutines.
There was a problem hiding this comment.
yeap I saw the same thing...
but it may worth to test it on windows and verify if it really catch false positives?
There was a problem hiding this comment.
See golang/go#64170. Based on that, one solution would be to create a custom version of AdjustTokenPrivileges that directly returns ERROR_NOT_ALL_ASSIGNED if applicable. Would be a bit hairy though. There may also be a way of doing it by inspecting the PreviousState (aka prevstate) parameter after the call. An alternative could be to query token state afterwards, but quite a bit of code also.
Per golang/go#41220, unfortunately GetLastError should not be used ("I don't know why we even define it").
There was a problem hiding this comment.
I remember seeing it fail before and I thought maybe it properly handles the case of one privilege. I tested now and it didn't so I switched to calling the syscall directly.
|
|
||
| ### Notes | ||
|
|
||
| - The workload attestor opens the access token of the calling process using [OpenProcessToken](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocesstoken). When the workload runs under a more privileged account than the SPIRE agent, this requires the agent process to have the [SE_DEBUG_NAME](https://learn.microsoft.com/en-us/windows/win32/secauthz/privilege-constants) privilege. The SPIRE agent attempts to enable this privilege at startup and logs a warning if it cannot. Ensure the account running the SPIRE agent has been granted this privilege if attestation of higher-privileged workloads is needed. |
sorindumitru
force-pushed
the
se-debug-enabled
branch
2 times, most recently
from
f5decde to
ff0d514
Compare
Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
sorindumitru
force-pushed
the
se-debug-enabled
branch
from
ff0d514 to
a0bf84f
Compare
Pull Request check list
Affected functionality
Windows workload attestor
Description of change
The OpenProcessToken API mentions that SE_DEBUG_NAME/SeDebugPrivilege (why two different names, windows???) privilege is required for opening the process token for a process running as a more privileged account (it does work from admin to admin users without this).
Which issue this PR fixes
fixes #7068