Skip to content

fix: use Windows ACLs for credential file permissions in backup import (#56923) - #56949

Open
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/windows-chmod-backup-credentials
Open

AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/windows-chmod-backup-credentials

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

os.chmod(path, 0o600) is a no-op on Windows — the file keeps its default 0o666 mode bits. This means credential-shaped files restored by hermes import are not tightened on Windows.

Changes

Add _restrict_file_permissions() helper to hermes_cli/backup.py:

  • POSIX: calls os.chmod(path, 0o600) as before
  • Windows: uses ctypes to call SetFileSecurityW / SetNamedSecurityInfoW with a DACL granting full access only to the current user and SYSTEM
  • Fallback: clears world-readable bits if Windows API calls fail

Replaces two bare os.chmod(target, 0o600) calls (lines 604, 648) with the cross-platform helper.

Why ctypes instead of pywin32

The pywin32 package (win32security) is an optional dependency. Using ctypes to call advapi32 directly avoids adding a hard dependency for a single function.

Fixes

Fixes #56923

os.chmod(path, 0o600) is a no-op on Windows — the file keeps its
default 0o666 mode bits. This means credential-shaped files restored
by 'hermes import' are not tightened on Windows.

Add _restrict_file_permissions() helper that:
- On POSIX: calls os.chmod(path, 0o600) as before
- On Windows: uses ctypes to set NTFS DACL granting full access
  only to the current user and SYSTEM
- Falls back to clearing world-readable bits if Windows API fails

Fixes NousResearch#56923
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage area/auth Authentication, OAuth, credential pools sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 2, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Competing with #56946 (@liuhao1024) for the same fix (#56923). This PR actively sets a Windows DACL (current-user + SYSTEM) via ctypes; #56946 instead skips the chmod on Windows and makes the test platform-aware, relying on the inherited NTFS ACL that already restricts new files to owner/SYSTEM/Administrators. Both are valid — leaving open for a maintainer to pick minimal (#56946) vs. belt-and-suspenders (#56949).

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the Windows credential-restore gap; current main still has the two bare chmod(0o600) calls at hermes_cli/backup.py:604 and :648.

Problems

  • The proposed SetNamedSecurityInfoW fallback at PR hermes_cli/backup.py:105-108 passes a security descriptor as pDacl and ignores the API return code. pDacl must be an ACL, so a primary-call failure can silently retain the inherited DACL.
  • The final fallback at PR hermes_cli/backup.py:116 repeats the chmod(0o600) operation the PR identifies as ineffective on Windows.
  • The diff adds no tests. Current tests/hermes_cli/test_backup.py:2471 asserts POSIX mode bits unconditionally, so it does not validate an NTFS ACL.

Suggested changes

  • Use correctly declared ctypes signatures, pass a real ACL to SetNamedSecurityInfoW (or retain one checked SetFileSecurityW path), check every return value, and release native resources.
  • Add a Windows-gated DACL verification test and restrict the existing mode-bit assertion to POSIX.

Automated hermes-sweeper review.

Comment thread hermes_cli/backup.py
str(path), SE_FILE_OBJECT, DACL_SECURITY_INFORMATION,
None, None, sd, None,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetNamedSecurityInfoW takes a PACL as its sixth argument, but sd is the PSECURITY_DESCRIPTOR returned by ConvertStringSecurityDescriptorToSecurityDescriptorW; also check its DWORD return value. As written, a failed primary call can reach this branch, silently fail again, and return as though the DACL was restricted.

Comment thread hermes_cli/backup.py
except OSError:
pass
return

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repeats the 0o600 chmod operation that the PR identifies as ineffective on Windows, so it cannot be a security fallback after an ACL API failure. Use a verified ACL-specific fallback or report the failure instead.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows] backup import test fails: os.chmod(..., 0o600) is a no-op on Windows

3 participants