[3/3] Add Windows SSH server support#480
Conversation
0eeeda6 to
4d63261
Compare
|
Rebased onto the latest |
|
@fdcastel Let me know when this is ready to review. It looks like it has a bunch of merge conflicts. |
4d63261 to
968c020
Compare
|
@EthanHeilman Everything set and ready to go! As always, feel free to request any adjustments you wish. Tests are passing on Windows Server 2025 and 2022! 🚀 |
|
Given the statement
We probably want to add Windows ARM64 to the integration tests |
- Rename workflow names to be distinct (Windows Server 2022 vs 2025) - Remove unnecessary -Encoding ascii from workflow Set-Content - Remove unused runneradmin password step from workflows - Fix copyright year in logpath_unix.go (2025 -> 2026) - Remove GetLogFilePathServer wrapper, call GetLogFilePath directly - Revert NOP variable extraction in sshcert.go NewFromAuthorizedKey - Simplify error message in sshcert.go type assertion failure - Keep release section at bottom of .goreleaser.yaml, add new fields - Remove redundant Windows fallbacks in openssh.go GetOpenSSHVersion - Add comment to PS1 test describe block - Add test coverage for ReadHome on Windows - Document Windows paths in docs/config.md
Working on it! |
|
Added initial support for tests on Windows 11 / ARM64. Changes:
P.S.: The first run of the ARM64 integration test timed out (10 min wasn't enough since Go isn't pre-installed on |
Interesting that the windows arm test is now faster than all the other integration tests. Edit: Ok, I see what you did. I meant add windows to the integration tests here opkssh/.github/workflows/ci.yml Lines 92 to 105 in 91ec2f7 |
|
Thanks for all your work on this. It is almost ready. Make sure to document that we support windows on the server support matrix in the readme |
- Rename workflow names to be distinct (Windows Server 2022 vs 2025) - Remove unnecessary -Encoding ascii from workflow Set-Content - Remove unused runneradmin password step from workflows - Fix copyright year in logpath_unix.go (2025 -> 2026) - Remove GetLogFilePathServer wrapper, call GetLogFilePath directly - Revert NOP variable extraction in sshcert.go NewFromAuthorizedKey - Simplify error message in sshcert.go type assertion failure - Keep release section at bottom of .goreleaser.yaml, add new fields - Remove redundant Windows fallbacks in openssh.go GetOpenSSHVersion - Add comment to PS1 test describe block - Add test coverage for ReadHome on Windows - Document Windows paths in docs/config.md
de0647d to
e0cec90
Compare
|
|
I'll give this another review once all the outstanding items are resolved. It is looking pretty close. Then I'll do a manual test on windows if I don't run into any errors, I'll merge it. Any reason not to merge when i get to that point? |
Nope, all good on my side! 👍🏻 That said, I’d love to test a rebased version of this PR on top of the other PRs I submitted yesterday -- just to do one last manual integration check. |
|
You want to do those PRs first? |
I just had that brilliant realization… right after I started submitting them 😅 You know... sleep deprivation + questionable decision-making. Jokes apart... Feel free to go with whatever flow works best for you. I’m happy to rebase anything if needed 👍 |
|
I'll merge them first. Reviewing and testing this PR is going to take a while. |
|
Please ring me here when you'd like a rebase onto the latest |
|
System PATH is now correctly configured on both Server 2022 and 2025. The Both Server 2022 and Server 2025 working (here 😉). |
1293929 to
c3a8a36
Compare
Found it! Fixing... |
Windows PowerShell 5.1's Set-Content -Encoding UTF8 writes a UTF-8 BOM (EF BB BF) which caused 'wrong number of arguments' errors when parsing the providers file. Fix 1: Strip UTF-8 BOM in NewTable() for robust config file parsing. Fix 2: Use [System.IO.File]::WriteAllText() in the installer to write UTF-8 without BOM, avoiding the issue at the source.
|
@EthanHeilman That Root CauseThe Here's how the BOM causes the parse error:
The actual provider entries on lines 2+ parse fine — the BOM only affects the first line. Fixes AppliedFix 1 — Go code (defensive, handles any BOM source): Strip the UTF-8 BOM at the start of // Strip UTF-8 BOM if present
if len(content) >= 3 && content[0] == 0xEF && content[1] == 0xBB && content[2] == 0xBF {
content = content[3:]
}This is important for robustness since users may also edit config files with Windows Notepad, which can re-add a BOM on save. Fix 2 — PowerShell installer (prevents the BOM from being written): Replaced # Before:
Set-Content -Path $providersPath -Value $providersContent -NoNewline -Encoding UTF8
# After:
[System.IO.File]::WriteAllText($providersPath, $providersContent, [System.Text.UTF8Encoding]::new($false))A test case for BOM-prefixed input was also added to |
|
Got it working! The auth_id policy file appears to be be case sensitive, but ssh converts Administrator@ to administrator@ but I had set the username in the auth_id to Administrator for some reason. Not sure if we want to fix this:
Defaulting to being case sensitive seems like the safest option. Let me know if you have a better idea Recording.2026-03-30.092912.mp4 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
main.go:583
isOpenSSHVersion8Dot1OrGreaterbuilds a version string likev9.5and then compares it withv8.1.0viasemver.Compare.golang.org/x/mod/semvertreatsv9.5as invalid (missing patch), so this comparison will likely always report "< v8.1.0" and emit warnings even on new OpenSSH versions. Normalize the extracted version to a full semver (e.g., append.0for the patch or parsemajor.minor.patchwhen available) before callingsemver.Compare.
version := "v" + matches[1] // semver requires that version strings start with 'v'
// OpenSSH doesn't use semantic versioning, but does use major.minor which after stripping the patch version can be compared using semver
if semver.Compare(version, "v8.1.0") >= 0 {
// if version is greater than or equal to v8.1.0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Nice! I’ll take care of these tonight (in a few hours). |
- Remove -AuthCmdUser parameter from installer; hard-code 'opksshuser' to match Go-side permissions model (addresses comments on lines 103, 190) - Fix readhome_windows.go SID error message to use actual resolved SID via ConvertSidToString instead of userObj.Uid (addresses comment on line 92) - Remove per-user Windows policy from docs/config.md since home policy is not yet supported on Windows (addresses comment on line 125) - Add Go integration tests step to Windows CI entry; Docker-based tests skip on Windows with t.Skip (addresses comment on line 175)
The parameter was removed from Install-OpksshServer.ps1 in the previous commit but the gha-windows.yml workflow still passed it.
Agreed. SUGGESTION: normalize the principal to lowercase at the entry points on Windows only, leaving the core engine untouched.
Considerations:
@EthanHeilman! I can jump on this now, or open a new issue and tackle it after this PR is merged. Just let me know what you’d prefer. |
The Go integration tests are Docker-based and skip on Windows (6 of 8 tests were SKIP). Remove the misleading 'Run integration tests (Windows - Go)' step. The Windows matrix entry now runs only the Pester tests, which are the actual Windows-specific integration tests. Also skip Install Go and Install dependencies for the Windows entry since Pester tests don't need them.
I believe SSH already does this. I don't think this is worth solving as it opens a whole can of worms.
That is tricky. Create a ticket to document this. |
Done in #504 👍🏻 |
…matrix Remove Windows from the integration test matrix — the Docker-based integration tests don't apply to Windows, so the matrix entry required if-guards on every step. Instead, add Pester tests as a step in the existing test-windows job. This keeps the integration test matrix clean (no conditionals) and the Windows Pester tests still run in CI under 'Windows Tests'.
|
Tests breaking post merge https://github.com/openpubkey/opkssh/actions/runs/23819884753/job/69429162670 |
Working on it. |
The Login step in the gha.yml workflow used 'go run main.go' which only compiles the single file main.go. After PR openpubkey#480 added platform-specific files (logpath_unix.go, logpath_windows.go), this caused a build error: main.go: undefined: GetLogFilePath The fix is to use 'go run .' which compiles all files in the package, including the platform-specific files with their build constraints.
|
@EthanHeilman Found the problem. Fix in #506. |
…506) fix(ci): use 'go run .' instead of 'go run main.go' in gha workflow The Login step in the gha.yml workflow used 'go run main.go' which only compiles the single file main.go. After PR #480 added platform-specific files (logpath_unix.go, logpath_windows.go), this caused a build error: main.go: undefined: GetLogFilePath The fix is to use 'go run .' which compiles all files in the package, including the platform-specific files with their build constraints.
##### [\`v0.14.0\`](https://github.com/openpubkey/opkssh/releases/tag/v0.14.0) Adds support for sshing into windows servers. Openssh 10.13 makes a breaking, non-backwards compatible change to how ssh certificates work, this breaks opkssh older than this release. This release creates a fix for this breaking change. ##### Changes - feat: update to openpubkey 0.23.0 [@ianroberts](https://github.com/ianroberts) ([#510](openpubkey/opkssh#510)) - fix(ci): use `go run .` instead of `go run main.go` in gha workflow [@fdcastel](https://github.com/fdcastel) ([#506](openpubkey/opkssh#506)) - \[3/3] Add Windows SSH server support [@fdcastel](https://github.com/fdcastel) ([#480](openpubkey/opkssh#480)) - refactor: unify MockUserLookup into shared test helper package. Closes [#439](openpubkey/opkssh#439). [@fdcastel](https://github.com/fdcastel) ([#495](openpubkey/opkssh#495)) - Update CLI documentation @[github-actions\[bot\]](https://github.com/apps/github-actions) ([#500](openpubkey/opkssh#500)) - feat: add --inspect-cert and --verbose flags to login command. Closes [#353](openpubkey/opkssh#353). [@fdcastel](https://github.com/fdcastel) ([#497](openpubkey/opkssh#497)) - docs: Add GitHub Actions integration guide. Closes [#481](openpubkey/opkssh#481) [@fdcastel](https://github.com/fdcastel) ([#492](openpubkey/opkssh#492)) - test: cover full printed output of opkssh inspect. Closes [#356](openpubkey/opkssh#356) [@fdcastel](https://github.com/fdcastel) ([#493](openpubkey/opkssh#493)) - Update CLI documentation @[github-actions\[bot\]](https://github.com/apps/github-actions) ([#498](openpubkey/opkssh#498)) - Add `logout` command to remove opkssh-generated SSH keys. Closes [#317](openpubkey/opkssh#317). [@fdcastel](https://github.com/fdcastel) ([#496](openpubkey/opkssh#496)) - Update CLI documentation @[github-actions\[bot\]](https://github.com/apps/github-actions) ([#490](openpubkey/opkssh#490)) - \[2/3] Add permissions command [@fdcastel](https://github.com/fdcastel) ([#479](openpubkey/opkssh#479)) - bug: ensure provider arg doesn't skip remote-redirect-uri [@EthanHeilman](https://github.com/EthanHeilman) ([#471](openpubkey/opkssh#471)) - \[1/3] Update GitHub Actions workflows and .gitignore [@fdcastel](https://github.com/fdcastel) ([#478](openpubkey/opkssh#478)) - docs: Add AWS EC2 setup guide for opkssh [@Rishang](https://github.com/Rishang) ([#467](openpubkey/opkssh#467)) ##### 🐛 Bug Fixes - fix(deps): Update docker/build-push-action action to v7 @[renovate\[bot\]](https://github.com/apps/renovate) ([#512](openpubkey/opkssh#512)) - Fix for openssh 10.13 breaking principals wildcard in SSH certificates [@EthanHeilman](https://github.com/EthanHeilman) ([#513](openpubkey/opkssh#513)) - fix(deps): Update zizmorcore/zizmor-action action to v0.5.2 @[renovate\[bot\]](https://github.com/apps/renovate) ([#488](openpubkey/opkssh#488)) - fix(deps): Update dependency golangci/golangci-lint to v2.11.2 @[renovate\[bot\]](https://github.com/apps/renovate) ([#486](openpubkey/opkssh#486)) - fix(deps): Update goreleaser/goreleaser-action action to v7 @[renovate\[bot\]](https://github.com/apps/renovate) ([#484](openpubkey/opkssh#484)) - fix(deps): Update goreleaser/goreleaser-action action to v7 @[renovate\[bot\]](https://github.com/apps/renovate) ([#477](openpubkey/opkssh#477)) - fix(deps): Update actions/setup-go action to v6.3.0 @[renovate\[bot\]](https://github.com/apps/renovate) ([#482](openpubkey/opkssh#482)) - fix(deps): Update zizmorcore/zizmor-action action to v0.5.0 @[renovate\[bot\]](https://github.com/apps/renovate) ([#451](openpubkey/opkssh#451)) - fix(deps): Update Docker @[renovate\[bot\]](https://github.com/apps/renovate) ([#464](openpubkey/opkssh#464)) ##### 🧰 Maintenance - Improve install script to make linter happy, fix typo [@EthanHeilman](https://github.com/EthanHeilman) ([#514](openpubkey/opkssh#514))
##### [\`v0.14.0\`](https://github.com/openpubkey/opkssh/releases/tag/v0.14.0) Adds support for sshing into windows servers. Openssh 10.13 makes a breaking, non-backwards compatible change to how ssh certificates work, this breaks opkssh older than this release. This release creates a fix for this breaking change. ##### Changes - feat: update to openpubkey 0.23.0 [@ianroberts](https://github.com/ianroberts) ([#510](openpubkey/opkssh#510)) - fix(ci): use `go run .` instead of `go run main.go` in gha workflow [@fdcastel](https://github.com/fdcastel) ([#506](openpubkey/opkssh#506)) - \[3/3] Add Windows SSH server support [@fdcastel](https://github.com/fdcastel) ([#480](openpubkey/opkssh#480)) - refactor: unify MockUserLookup into shared test helper package. Closes [#439](openpubkey/opkssh#439). [@fdcastel](https://github.com/fdcastel) ([#495](openpubkey/opkssh#495)) - Update CLI documentation @[github-actions\[bot\]](https://github.com/apps/github-actions) ([#500](openpubkey/opkssh#500)) - feat: add --inspect-cert and --verbose flags to login command. Closes [#353](openpubkey/opkssh#353). [@fdcastel](https://github.com/fdcastel) ([#497](openpubkey/opkssh#497)) - docs: Add GitHub Actions integration guide. Closes [#481](openpubkey/opkssh#481) [@fdcastel](https://github.com/fdcastel) ([#492](openpubkey/opkssh#492)) - test: cover full printed output of opkssh inspect. Closes [#356](openpubkey/opkssh#356) [@fdcastel](https://github.com/fdcastel) ([#493](openpubkey/opkssh#493)) - Update CLI documentation @[github-actions\[bot\]](https://github.com/apps/github-actions) ([#498](openpubkey/opkssh#498)) - Add `logout` command to remove opkssh-generated SSH keys. Closes [#317](openpubkey/opkssh#317). [@fdcastel](https://github.com/fdcastel) ([#496](openpubkey/opkssh#496)) - Update CLI documentation @[github-actions\[bot\]](https://github.com/apps/github-actions) ([#490](openpubkey/opkssh#490)) - \[2/3] Add permissions command [@fdcastel](https://github.com/fdcastel) ([#479](openpubkey/opkssh#479)) - bug: ensure provider arg doesn't skip remote-redirect-uri [@EthanHeilman](https://github.com/EthanHeilman) ([#471](openpubkey/opkssh#471)) - \[1/3] Update GitHub Actions workflows and .gitignore [@fdcastel](https://github.com/fdcastel) ([#478](openpubkey/opkssh#478)) - docs: Add AWS EC2 setup guide for opkssh [@Rishang](https://github.com/Rishang) ([#467](openpubkey/opkssh#467)) ##### 🐛 Bug Fixes - fix(deps): Update docker/build-push-action action to v7 @[renovate\[bot\]](https://github.com/apps/renovate) ([#512](openpubkey/opkssh#512)) - Fix for openssh 10.13 breaking principals wildcard in SSH certificates [@EthanHeilman](https://github.com/EthanHeilman) ([#513](openpubkey/opkssh#513)) - fix(deps): Update zizmorcore/zizmor-action action to v0.5.2 @[renovate\[bot\]](https://github.com/apps/renovate) ([#488](openpubkey/opkssh#488)) - fix(deps): Update dependency golangci/golangci-lint to v2.11.2 @[renovate\[bot\]](https://github.com/apps/renovate) ([#486](openpubkey/opkssh#486)) - fix(deps): Update goreleaser/goreleaser-action action to v7 @[renovate\[bot\]](https://github.com/apps/renovate) ([#484](openpubkey/opkssh#484)) - fix(deps): Update goreleaser/goreleaser-action action to v7 @[renovate\[bot\]](https://github.com/apps/renovate) ([#477](openpubkey/opkssh#477)) - fix(deps): Update actions/setup-go action to v6.3.0 @[renovate\[bot\]](https://github.com/apps/renovate) ([#482](openpubkey/opkssh#482)) - fix(deps): Update zizmorcore/zizmor-action action to v0.5.0 @[renovate\[bot\]](https://github.com/apps/renovate) ([#451](openpubkey/opkssh#451)) - fix(deps): Update Docker @[renovate\[bot\]](https://github.com/apps/renovate) ([#464](openpubkey/opkssh#464)) ##### 🧰 Maintenance - Improve install script to make linter happy, fix typo [@EthanHeilman](https://github.com/EthanHeilman) ([#514](openpubkey/opkssh#514))
Based on #479 -- Closes #370
Adds full support for running
opksshas an SSHAuthorizedKeysCommandon Windows OpenSSH servers, including installation scripts, platform-specific path handling, and integration test workflows.This is the third and final PR splitting the work from #389.
How to test
Tested on
Changes
Windows installation scripts (
scripts/windows/)Install-OpksshServer.ps1— Fully automated installer for opkssh on Windows SSH servers. Handles:sshd_configforAuthorizedKeysCommandopksshuserservice account (for pre-Server 2025 systems)AuthorizedKeysCommandUsersupport (System account on 8.9+)Uninstall-OpksshServer.ps1— Clean uninstaller that reverses all installation changes.Test-OpksshInstallation.ps1— Post-installation validation script.test/Install-OpksshServer.Tests.ps1— Pester tests for the installer.Platform-specific paths and logging
logpath_unix.go/logpath_windows.go— Platform-specific log file paths (/var/log/opkssh.logon Unix,%ProgramData%\opk\opkssh.logon Windows).main.go: Updated verify command to use platform-independent configuration paths viaGetSystemConfigBasePath().OpenSSH version handling
main.go(isOpenSSHVersion8Dot1OrGreater): Updated version parser to handle Windows OpenSSH version format (e.g.,OpenSSH_for_Windows_9.5p2, LibreSSL 3.8.2) in addition to standard Unix format.internal/sysdetails/openssh.go: Enhanced OpenSSH version detection with OS-specific methods.internal/sysdetails/os.go: Added OS name detection utility.ReadHome for Windows
commands/readhome_windows.go: Windows implementation for reading user home directory policy files, resolving user profile paths from the Windows registry.Build and release
.goreleaser.yaml:Policy and enforcement
policy/enforcer.go: Made plugin policy directory path platform-aware usingGetPluginPolicyDir().SSH certificate improvements
sshcert/sshcert.go: Improved error messages when parsing SSH authorized keys, including actual type information and truncated key data for easier debugging.GitHub Actions
gha-windows.yml: Integration test workflow for Windows Server 2022. Tests the full SSH flow: install OpenSSH Server → build opkssh → configure AuthorizedKeysCommand → authenticate with GitHub OIDC → SSH login.gha-windows-2025.yml: Same integration test for Windows Server 2025.Related