fix(core): enforce owner-only Windows ACLs on sensitive files and dirs - #3495
Open
pkhodade-NV wants to merge 2 commits into
Open
pkhodade-NV wants to merge 2 commits into
pkhodade-NV wants to merge 2 commits into
Conversation
set_dir_owner_only/set_file_owner_only were unconditional no-ops on Windows, so the CLI's mTLS client private key, OIDC/edge tokens, cached SSH keys, and the gateway's key-encryption key relied entirely on inherited NTFS ACLs with no OpenShell-applied restriction. Apply an owner-only DACL via SetEntriesInAclW/SetNamedSecurityInfoW with PROTECTED_DACL_SECURITY_INFORMATION to strip inherited ACEs, matching the 0700/0600 guarantee already provided on Unix. is_file_permissions_too_open now also works on Windows instead of being Unix-only, closing the detection gap alongside the prevention gap. Signed-off-by: Prashant Khodade <pkhodade@nvidia.com> (cherry picked from commit 71560e947f85819efbcddf70ddda94befab62b0b)
has_foreign_trustee conflated a NULL DACL with an unreadable/invalid ACL and returned Some(false) (not too open) for both. Per the Win32 contract, a NULL DACL means the object grants full access to everyone -- the most permissive state possible -- so it must be flagged as too open. Split the null and invalid-ACL branches: null now returns Some(true), invalid ACL keeps the existing unreadable-ACL fallback (None, which the caller maps to false via unwrap_or). Adds a regression test that constructs a real NULL DACL via a SetNamedSecurityInfoW helper confined to the windows_acl module, consistent with the existing unsafe-FFI confinement in that module. Found by CodeRabbit review on MR !113. Signed-off-by: Prashant Khodade <pkhodade@nvidia.com> (cherry picked from commit 46e635a4ef1d6937cdb088f46aa85baee3d6ad28)
pkhodade-NV
requested review from
a team,
derekwaynecarr,
mrunalp and
sjenning
as code owners
September 20, 2026 15:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
set_dir_owner_only/set_file_owner_onlyinopenshell-core/src/paths.rswere unconditional no-ops on Windows (documented as such), so locally-stored CLI credential material (mTLS client private key, OIDC/edge tokens, cached SSH keys) and the gateway's key-encryption key relied entirely on inherited NTFS ACLs, with no OpenShell-applied restriction.is_file_permissions_too_open(the one available detection mechanism) was also Unix-only, so there was no way to audit for this after the fact either.Related Issue
No linked issue — this is a security-sensitive, localized fix to platform-specific permission-hardening helpers already documented as required (the doc comments on these functions state the intent plainly; only the Windows implementation was missing).
Changes
paths.rs:set_dir_owner_only/set_file_owner_onlynow apply a real owner-only Windows ACL viaSetEntriesInAclW/SetNamedSecurityInfoWwithPROTECTED_DACL_SECURITY_INFORMATION, which strips inherited ACEs from the parent directory — the actual source of the unintended access. Directories additionally propagate the ACE to children (SUB_CONTAINERS_AND_OBJECTS_INHERIT).is_file_permissions_too_openis no longer Unix-only; on Windows it inspects the DACL viaGetNamedSecurityInfoWand flags any ACE granted to a trustee other than the current user.windowscrate (already a workspace dependency, used by the MXC driver's ETW consumer) as a Windows-only dependency ofopenshell-core, with the additionalWin32_Security*/Win32_Storage_FileSystem/Win32_System_Memory/Win32_System_SystemServices/Win32_System_Threadingfeatures needed for the ACL APIs. TheunsafeFFI surface is confined to a privatewindows_aclsubmodule, matching the existing precedent inopenshell-driver-mxc.openshell-driver-db-credstore's key-encryption-key generation (previously#[cfg(unix)]-only, so the gateway's master key-encryption key had no Windows test coverage at all).openshell-server/src/persistence/sqlite.rs.Cargo.lockfiles (governance-interceptor,supervisor-middleware-content-guard) since both path-depend onopenshell-core; diffs are purely additive (newwindows-*crate entries only, no other package bumped).No caller changes were needed in
openshell-bootstrap/mtls.rs,edge_token.rs,oidc_token.rs, oropenshell-cli/ssh.rs— they already route through the fixedpaths.rsfunctions.Testing
paths.rsmirroring the four existing Unix tests.icacls: a restricted file gets a single owner-only ACE (user:(F)), a restricted directory getsuser:(OI)(CI)(F)— no leftover SYSTEM/Administrators/Users entries from inheritance.cargo test -p openshell-driver-db-credstore --target x86_64-pc-windows-msvc generated_key_encryption_key_file_is_owner_onlypasses on Windows.Checklist
Originally opened as GitLab MR !113 against our internal mirror; re-opened here against
windowsfor upstream review.