Repository navigation
Install and enforce the workspace policy on Windows - #1262
billiam-wang wants to merge 13 commits into
Conversation
Explicit ug configure now installs the workspace policy at /etc/unitygateway/managed-settings.json on Linux and macOS, using a non-interactive-sudo-capable writer. Unchanged policy writes nothing, and no-config or feature-disabled removes the installed file. A failed install raises ManagedPolicyInstallDenied with an actionable message. Launches keep reading the per-user cache and never touch the machine file. Co-authored-by: Isaac <no-reply@databricks.com>
Launches read the policy installed at /etc/unitygateway/managed-settings.json, refuse a selection for another workspace, and refresh at most every five minutes using user-owned stamps in ~/.ucode/refresh_ts.json. Non-root launches never install or remove the policy. The per-user cache remains only for Windows, UG_SKIP_OS_SETTINGS, and --user. Configure restores the saved workspace on a denied install, and other agents refuse early on a machine managed elsewhere. Also adds ug export, dry-run workspace adoption, and the CUJ recorder and harness reading the installed policy. Co-authored-by: Isaac <no-reply@databricks.com>
`ug revert` deletes /etc/unitygateway/managed-settings.json with sudo; nothing else writes that file. Without sudo it warns and keeps it, and Windows, UG_SKIP_OS_SETTINGS and `--user` leave it alone. Test cleanup keeps a backstop removal in its own finally, so a failed or skipped revert can no longer leave the policy behind for later cases. Co-authored-by: Isaac <no-reply@databricks.com>
# Conflicts: # docs/managed-workspace-policy.md # src/ucode/managed_config.py # tests/test_managed_config.py
Windows now holds the installed workspace policy like Linux and macOS: %ProgramData%\UnityGateway\managed-settings.json, readable by every user. - An elevated `ug configure` writes, replaces, or removes it atomically; the UnityGateway folder is secured to Administrators (standard users read only) and links are refused. An elevated `ug revert` removes it. - A non-elevated configure fails with an administrator hint; a launch never elevates, and a non-elevated launch applies the fetched policy for that run when nothing is installed. - Launches on Windows read and enforce only the ProgramData file. - The per-user cache remains only for UG_SKIP_OS_SETTINGS and --user. - The integration preflight and cache-path helpers include the Windows policy path. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
/ug-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if current_os() is OS.WINDOWS: | ||
| hint = ( | ||
| "Run `ug configure` from an elevated terminal (Run as administrator), or ask " | ||
| "your IT team or an administrator to run it." | ||
| ) |
There was a problem hiding this comment.
will this break current non-admin windows users? A non-elevated ug configure used to succeed via the per-user cache; now it raises ManagedPolicyInstallDenied. Once installable, load_managed_state also never reads the existing per-user cache. can we fall back to the per-user cache when not elevated?
|
|
||
| On Windows the path is the protected ProgramData file, never the POSIX one. | ||
| """ | ||
| return current_os() in (OS.LINUX, OS.MACOS, OS.WINDOWS) |
There was a problem hiding this comment.
what happens if a standard user creates ProgramData\UnityGateway\managed-settings.json before any admin install? ProgramData lets users create subfolders, and _installed_config trusts the file without checking owner or ACL. On a shared machine, user A can plant {workspace: <B's workspace>, config: <hooks/MCP>}. When B launches, the non-admin write fails and the code continues with the installed policy, so A's hooks run as B. can we check that Administrators or SYSTEM own the file and folder before trusting it?
|
|
||
| def test_only_the_program_data_file_is_read(self, tmp_path): | ||
| # A POSIX-style path under a drive root is never consulted on Windows. | ||
| planted = tmp_path / "etc" / "unitygateway" / "managed-settings.json" |
There was a problem hiding this comment.
does this test anything? MANAGED_CONFIG_PATH is already patched to the ProgramData path, so nothing reads tmp_path/etc/... and the test passes whether or not a POSIX path would be read. can we drop it, or patch the real POSIX path instead?
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Removal hands the replace script the exact bytes of the installed file, so a file with a BOM or CRLF endings still matches its byte-for-byte check. Add a regression test and a comment on why the copy is binary. Co-authored-by: Isaac <no-reply@databricks.com>
Polly AI ReviewBlocking issuesTwo admin-only-write violations are listed under Security vulnerabilities. Security vulnerabilities
For both fixes, extend the native Windows permission-test pattern in Non-blocking notes
ApproachReusing the Windows replacement path and extracting the Codex folder-hardening helper is consistent with existing patterns. The security requirement needs an exact allowlisted DACL and explicit permission validation independent of content equality. SummaryDiff size (computed): 10 files; +461 / -99 lines (560 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesThe PR adds Windows machine-policy installation and enforcement, elevation guidance, revert handling, integration-runner safeguards, documentation, and focused test support. The shared ACL-helper extraction directly supports this outcome. The implementation is proportional to the requested Windows increment. No substantial unrelated refactor or unnecessary product scaffolding was found. The main reduction opportunity is overlapping test coverage, assessed below. TestsTest-by-test assessmentStatic assessment only: no tests were executed. The frozen diff’s checksum matched, and both trusted policy files were read in full. The trusted checkout does not contain the supplied base/head commits or the complete stacked machine-policy implementation. Consequently, unchanged caller behavior and additional consumers of the modified fixture could not be fully verified.
Existing Codex tests cover shared ACL-command failure, directory-link rejection, and native Windows ownership/access checks. Those patterns are sufficient foundations; a live-agent E2E is unnecessary for these filesystem guarantees. The demonstrated coverage gaps are insecure pre-existing explicit ACLs and byte-identical insecure installations. The requested regressions are described under Security vulnerabilities. Native Windows ACL and junction behavior remains unverified; Linux-hosted command stubs do not establish it. Windows For human verification, use a disposable Windows machine:
ScopeNo clearly unrelated changes were found. Windows policy handling, runner safeguards, integration-state classification, documentation and the shared ACL-helper extraction all support the stated outcome. The unavailable stacked reader implementation limits verification of policy trust on read; it is an unresolved security question, not evidence of unrelated scope. Automated review by Polly · workflow run |
- The --agent refusal says the selection stays on the installed workspace and to rerun configure from a terminal with sudo access. - The launch refusal tells skip-mode users to switch without UG_SKIP_OS_SETTINGS, which is the only way the policy can change. - _launch_workspace_state resolves the target workspace once. - The configure integration test checks the policy folder is root-owned. Co-authored-by: Isaac <no-reply@databricks.com>
🥞 Stacked PR
Use this link to review incremental changes.
Worktree: ug-policy-windows | Branch: mdm-policy-windows
What did you change, and why?
Third PR in the stack, based on #1260. It brings the workspace policy to Windows, with the same rules as Linux and macOS:
ug configure, run elevated, installs, replaces or removes%ProgramData%\UnityGateway\managed-settings.json. Every user can read it; only administrators can write it.ug revert, run elevated, removes it.The
UnityGatewayfolder is locked down like Codex'sProgramDatafolders; links are refused.UG_SKIP_OS_SETTINGSand--userkeep the per-user cache.How do you know it works?
Unit tests cover install, replace, remove, enforcement and revert on Windows. The icacls steps are stubbed, so Windows CI is their first real run.
This pull request and its description were written by Isaac.