Adds support to OpenSSH Servers on Windows.#389
Conversation
fdcastel
force-pushed
the
issue-370-pr
branch
from
a4475de to
66de4f2
Compare
|
Fixed failing test. Rebased onto the latest |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
UPDATE: This version no longer uses Windows containers. See below. |
fdcastel
force-pushed
the
issue-370-pr
branch
from
697626f to
7a7b3ba
Compare
fdcastel
force-pushed
the
issue-370-pr
branch
from
b103295 to
f7b52ca
Compare
|
|
@fdcastel This is looking really nice! I saw you were doing some refactoring of permissions between windows and linux. OPKSSH doesn't handle file permissions in the most organized way. We have opkssh/commands/config/client_config.go Line 96 in 618effe If you ended up refactoring my current code into unified permissions struct across all OPKSSH, I would not object. I've been thinking about doing some similar and it seems you probably have to do it to support windows. |
That's great news! 😄 |
|
BTW: should we consider removing the Docker container from the I’d also say the existing workflows could benefit from a little cleanup 😅. |
|
@EthanHeilman I'm considering adding a new The idea is to centralize all platform-specific permission settings within the application itself. This would simplify our installer scripts (on both Linux and Windows), which currently handle much of this logic, thereby reducing code duplication and keeping all permission management contained within the app. It could also be useful for quickly repairing user installations that (for any reason) may have become corrupted. What do you think? |
|
Sure! This entire PR (which is getting quite large, by the way) is meant to serve as a proof of concept for now. My intention isn’t to merge it as-is. Once everything is working properly and looks good, I plan to split it into several smaller PRs and rebase them as needed -- e.g. to adapt to new changes like #388. It will likely take me a few more weeks to finish, but progress is looking very promising. There’s still quite a bit of work to do, though (trying to build a good abstract layer for permissions in both Posix and Windows platforms). |
|
@EthanHeilman Just a quick follow-up: I had to put this work on hold for a while since things are hectic here toward year-end. 😅 But, just to reinforce, I’m still really interested in getting this done, and I expect to pick it back up soon (most likely at the beginning of January). I’m currently refactoring the work from this PR to make it simpler and more robust across both platforms. The updated work is happening here: https://github.com/fdcastel/opkssh/tree/issue-370 (note: this is not the same source branch as this PR).
Also, I noticed you’re doing some merges here. If you’d like, I can rebase this PR when I’m back. Best regards, and happy holidays! Fabio. |
|
Happy holidays! Looking forward to working with you when you get back in early Jan |
TL;DRWhat about adding a Long versionI’m upgrading the audit command to support Windows as well. In my earlier code -- based on v0.10, before the audit command existed (no in this PR) -- I added a new
The The other two, I believe, can be handled with a single @Basti-Fantasti @EthanHeilman Opinions? |
|
On second thought… the For installers, it’s better to explicitly run We could refactor this so that the permissions check becomes a smaller component of the overall audit process. |
fdcastel
force-pushed
the
issue-370-pr
branch
from
3ab5b6d to
b51ca5f
Compare
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| - name: Publish GitHub release | ||
| run: gh release edit "${{ github.ref_name }}" --draft=false --repo "${{ github.repository }}" |
Check failure
Code scanning / zizmor
code injection via template expansion Error
|
I’ve pushed the latest version and now I would love reviews and opinions. Testing instructions have been updated. This version has been tested on Windows Server 2022 and 2025, but it should also work on any Windows edition that supports Microsoft’s OpenSSH server. What’s new:
Please take a look and let me know what you think. |
I think a command like this could be useful. Feel free to create a ticket for it with a description of what it would do and I'll provide feedback |
Agreed. But I will also need this functionality in this current PR. My design relies on this command to eliminate duplicated code in the installation scripts, centralizing the permission logic in a single place (inside the application). This logic may be reused by the This approach is already used in the Windows installer. And the plan is to extend it to the Linux installer as well. |
|
If you’re good with this direction, I can split this PR in two:
and then rebasing this PR on top of the first. |
|
That said, this only makes sense if the group find the Alternatively, if a little permission code duplication between the scripts and the app is tolerable -- and a need for a dedicated “fix” command doesn't exists -- I could remove this code entirely. I’m just looking for guidance on what the team thinks is best for the project. After giving it some thought, my preference would be to keep the |
Sorry for my delayed response. I'm a bit late to the party 😄 |
|
Thanks, guys, for the feedback! I’m currently abroad, but I plan to revisit this next week (making a PR for the new P.S.: That said, we’re still looking for more Windows testers! 😅 |
| } else if err != nil { | ||
| return nil, fmt.Errorf("failed to read /etc/passwd: %v", err) | ||
| if isWindows() { | ||
| fmt.Fprint(a.ErrOut, "skipping user policy audit on Windows (no /etc/passwd)\n") |
There was a problem hiding this comment.
Makes sense for right now to not have to support this, but there is probably a way to get all windows users on a server
There was a problem hiding this comment.
I will investigate 👍🏻
|
|
||
| // Providers dir | ||
| if _, err := ops.Stat(providersDir); err != nil { | ||
| if err := ops.MkdirAllWithPerm(providersDir, 0750); err != nil { |
There was a problem hiding this comment.
We should a file that has the correct permissions for each file type. Hardcoding them this way is likely to cause bugs down the road. I started this anti-pattern, but it would be great to fix it in this PR.
There was a problem hiding this comment.
I THINK I understand what you want 😅.
But... If you could elaborate a bit more -- with a few examples -- I’d be glad to look into it further!
There was a problem hiding this comment.
Something like this, not exactly this, I haven't really thought it through, but in this sort of ballpark
type PermInfo struct {
Mode fs.FileMode
Owner string
Group string
MustExist bool
}
var PermsEum = struct {
HOME_AUTHID FileInfo
ETC_AUTHID FileInfo
ETC_CONFIG_YML FileInfo
}{
HOME_AUTHID: PermInfo {
Mode: fs.FileMode(0600),
Owner: "",
Group: "",
MustExist: false
},
ETC_AUTHID: PermInfo {
Mode: fs.FileMode(0640),
Owner: "opksshuser",
Group: "opksshuser",
MustExist: true
},
ETC_CONFIG_YML: PermInfo {
Mode: fs.FileMode(0640),
Owner: "opksshuser",
Group: "opksshuser",
MustExist: true
},
}Something close to this exists in Perms Checker, but missing details
https://github.com/openpubkey/opkssh/blob/main/policy/files/permschecker.go#L28-L37
You'd probably want Perminfo to support Linux and Windows permissions.
Introduces a PermInfo struct in policy/files that defines the expected
mode, owner, group, and existence requirements for each opkssh resource
type (SystemPolicy, HomePolicy, ProvidersDir, PluginsDir, PluginFile).
Platform-specific values are provided via build-tagged files
(perminfo_unix.go and perminfo_windows.go) so the correct owner
('root' vs 'Administrators') is selected at compile time.
Replaces hardcoded permission values scattered across
commands/permissions.go, commands/audit.go, and commands/platform.go
with references to the centralized files.RequiredPerms definitions.
Addresses review feedback from EthanHeilman on PR openpubkey#389.
Replaces the 'skipping user policy audit on Windows' stub with actual user profile enumeration via the Windows registry ProfileList at HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList. New files: - commands/audit_windows_profiles.go: reads ProfileList registry keys, filters to real user SIDs (S-1-5-21-*), resolves usernames via os/user.LookupId, and returns profile paths. - commands/audit_enum_windows.go: Windows enumerateUserHomeDirs() method that delegates to getHomeDirsFromProfileList(). - commands/audit_enum_unix.go: Unix enumerateUserHomeDirs() method that reads /etc/passwd (extracted from audit.go). Changes: - commands/audit.go: unified user enumeration loop that calls the platform-specific enumerateUserHomeDirs(); removed isWindows() skip. - NewAuditCmd no longer defaults SkipUserPolicy to true on Windows. - commands/audit_windows_test.go: updated tests to verify enumeration runs and SkipUserPolicy flag works. No new dependencies - uses golang.org/x/sys/windows/registry which is already available via the existing golang.org/x/sys dependency.
The isWindows() function in commands/platform.go is no longer called after the audit user enumeration was refactored to use platform-specific build-tagged files. The golangci linter correctly flagged this as unused.
- Create permissions_mocks_test.go with shared mockFilePermsOps and mockACLVerifier types used by both Unix and Windows tests - Remove duplicated mockOpsNW/mockVerifierNW and mockOps/mockVerifier - Fix test file naming: rename *_test_windows.go to *_windows_test.go so Go's test runner properly discovers the test functions
- Refactor SetupAuditCmdMocks to use platform-aware paths (policy.SystemDefaultPolicyPath) instead of hardcoded Unix paths - Make etcPasswdContent parameter optional (empty string skips writing) - Update audit_windows_test.go to call SetupAuditCmdMocks instead of duplicating AuditCmd setup inline - Fix test expectations to use platform-aware policy path strings
- Create permcheck.go with CheckFilePermissions() that centralises the existence, mode/ownership, and ACL checks shared by audit and permissions - Update audit.go auditPolicyFileWithStatus to use CheckFilePermissions - Update permissions.go runPermissionsCheck to use CheckFilePermissions - Both commands now use the same checking logic, ensuring consistent results
Replace the 'not supported' stub with a full implementation that: - Validates the username format - Looks up the user via user.Lookup to get their SID and home directory - Reads the policy file from <HomeDir>\.opk\auth_id - Verifies file ownership by comparing the file owner SID against the expected user SID using the existing ACL verification infrastructure - Reports any ACL problems found on the file
Add the standard Apache 2.0 license header to permissions.go and fileperms_ops.go, consistent with all other source files.
Add a comparison table and usage guidance to docs/audit.md explaining when to use 'opkssh audit' vs 'opkssh permissions check' and how they relate to each other.
Add TestAuditAndPermissionsCheckConsistency to verify that the audit command and CheckFilePermissions report the same results for a given system policy file. Also add TestAuditAndPermissionsCheckBadPerms (Unix-only) to verify both detect incorrect mode bits consistently.
- Remove unused expectedSystemACL() from platform.go (golangci-lint unused) - Fix gofmt formatting in audit_permissions_test.go - Fix CmdRunner mock in SetupAuditCmdMocks to return 'opksshuser' instead of 'opkssh', matching RequiredPerms.SystemPolicy.Group on Linux. This was exposed by CheckFilePermissions now passing owner/group to CheckPerm.
|
I pushed the latest improvements and fixes, as well the suggestions from the reviews. I'll start to split this into smaller PRs.
|
4 participants
How to test
Tested on:
Long version (grab a ☕!)
I began migrating the existing
scripts/install-linux.shcode to PowerShell, aiming to maintain as much fidelity as possible.During the first run, I quickly encountered an issue with hardcoded POSIX filesystem paths. I adapted these to Windows paths to the best of my (admittedly limited) knowledge of Go 😅 (kudos to Claude Sonnet 4.5 for the assist!)
Some parts of the code depend on specific file permissions to run correctly. Initially, I tried to preserve this behavior by emulating the same permissions through Windows ACLs, hoping to keep the original Go code untouched. However, this turned out to be an ungrateful and impractical task, so I eventually abandoned that path.
A few smaller issues also came up, such as differences in version string formats between Windows builds. I adjusted the Go code to handle those as well.
Then I discovered a major difference in the OpenSSH Server shipped with Windows Server 2025: This PR, merged on March 26, 2021, enables the use of
AuthorizedKeysCommandUser = System, eliminating the need to create a dedicated user for runningopkssh.Browsing the Win32-OpenSSH repository, I couldn’t find a clear mapping between specific PRs and releases. However, based on the PR’s merge date and the release history
it’s reasonable to assume that versions
8.9.0.0and up includes this change.Unfortunately, it seems that all versions shipped with Windows prior to Server 2025 are earlier than this one. I added a safeguard to detect this condition and alert the user, instructing them to use the appropriate CLI option to create the
opksshuserwhen necessary.Finally, modifying the system PATH on Windows is not exactly straightforward. I updated the script to handle PATH expansion correctly, avoiding one of Windows’ many quirks.
And so… this is it! Now,
But... hey! It's working. 😄
To Do:
$env:ProgramData/opk) -- Is it acceptable?opksshconfiguration files -- is this necessary on Windows?-NoHomeProfileinstaller option)Fix #370.