[2/3] Add permissions command#479
Conversation
a850074 to
ca09746
Compare
|
Rebased onto the latest |
a6edc5a to
7726732
Compare
|
Rebased onto the latest @EthanHeilman: this one’s up next in the queue 😉 I’m happy to take care of any changes or fixes you’d like. |
|
@fdcastel Looking forward to reviewing this. I won't have time until next week, 55 files is a lot of files to review =) |
|
I completely understand. Yes, there are quite a few changes related to infrastructure updates and other refactors that I picked up along the way. I tried to split them between this PR and the next one. If you have any suggestions, I could look into breaking this down into smaller PRs. I’ll try to explore an approach along those lines, if time permits. |
EthanHeilman
left a comment
There was a problem hiding this comment.
Not a complete review, but all the time I had today
- Add new 'opkssh permissions' command with check/fix/install subcommands for managing file permissions and ACLs on both Linux and Windows - Refactor audit command to use shared permission checking infrastructure - Introduce PermInfo struct for centralized file permission definitions - Add ACL verification abstraction (Unix file modes + Windows ACLs) - Extract platform-specific user enumeration for audit command - Add Windows user profile enumeration for audit - Move platform-specific test code to build-tagged files - Refactor policy paths to be platform-independent using GetSystemConfigBasePath() - Split ReadWithSudoScript into platform-specific files - Add comprehensive tests for permissions command - Add audit + permissions consistency documentation - Export RequiredPolicyDirPerms from plugins package
- Fix copyright year to 2026 in all new files - Add full Apache license headers to elevate_unix.go and elevate_windows.go - Use 0o prefix for octal file permission literals (0o640, 0o750, etc.) - Refactor permissions.go: use PermissionsCmd struct with Out/ErrOut/In fields instead of package-level variables and disconnected functions - Add --json flag to permissions check and fix subcommands - Rename etcPasswdRow to userHomeEntry (generic across platforms) - Add explanatory comments for silently skipped errors in audit_windows_profiles.go - Add unit tests for enumerateUserHomeDirs on both Unix and Windows - Update all test files to use struct-based PermissionsCmd API
- Fix copyright year to 2026 in all new files - Add full Apache license headers to elevate_unix.go and elevate_windows.go - Use 0o prefix for octal file permission literals (0o640, 0o750, etc.) - Refactor permissions.go: use PermissionsCmd struct with Out/ErrOut/In fields instead of package-level variables and disconnected functions - Add --json flag to permissions check and fix subcommands - Rename etcPasswdRow to userHomeEntry (generic across platforms) - Add explanatory comments for silently skipped errors in audit_windows_profiles.go - Add unit tests for enumerateUserHomeDirs on both Unix and Windows - Update all test files to use struct-based PermissionsCmd API
1cf0c26 to
77648e4
Compare
|
Rebased onto the latest
|
|
Probably too big of an ask, but the permissions issue adds a lot of complex and files, is there anyway to create a mockable files struct that hides and abstracts away all the permission and filesystem details. |
I understand. No problem! I’m a bit tight on schedule right now, but give me a few days and I’ll come back with something. Also, if you wish/have any suggestions on how to split this PR into smaller sub-PRs, I’m all ears. 😄 |
Consolidate Fs (afero.Fs), FilePermsOps, PermsChecker, and ACLVerifier into a single mockable files.FileSystem interface. This hides platform- specific permission and filesystem details behind one abstraction. Changes: - New policy/files/filesystem.go: FileSystem interface with 13 methods covering file I/O, permission mutations, permission checking, and ACL verification. DefaultFileSystem delegates to existing implementations. - PermissionsCmd: Replace Fs+Ops+ACLVerifier fields with single FileSystem field. - AuditCmd: Replace filePermsChecker+aclVerifier with FileSystem field (Fs kept for backward-compatible file reads). - CheckFilePermissions: Simplified from 5 params to 3 (FileSystem, path, permInfo). - All test mocks consolidated into one mockFileSystem struct.
Tests using in-memory filesystems need a mock CmdRunner since the real 'stat' command cannot operate on afero.MemMapFs paths. - Add FileSystemOption/WithCmdRunner to configure the command runner used by the underlying PermsChecker - Update all test constructors to use WithCmdRunner - Fix gofmt struct field alignment in AuditCmd
|
@EthanHeilman I've implemented an unified mockable This is just a first attempt. If you're not happy with it, we can always roll it back and try a different approach. Do not hesitate to suggest alternative paths -- I’m totally open to them! Summary of changesNew:
The Refactored Refactored Refactored Simplified test mocks — A single Files changed (12 files)
All existing tests pass on both platforms. CI is green. |
|
Awesome feedback, @EthanHeilman!! And great catches, too. I’ll start working on these shortly. |
…ve dead code - Rename AuditCmd.FileSystem to Fs (merging with old afero.Fs field) - Rename RequiredPerms.ProvidersDir to Providers (it's a file, not dir) - Add RequiredPerms.Config for server_config.yml permission checks - Add config.yml permission checking to permissions check/fix commands - Fix providers to be treated as a file (chmod/chown) not a directory - Pass actual perm param in mock MkdirAll/Chmod/WriteFile methods - Pass RequiredPerms.PluginsDir.Group in plugins dir permission check - Rename verify_test_unix.go to verify_unix_test.go (Go test naming) - Remove acl_unit_test_nonwindows.go (skip-only test, no value) - Remove expectedSystemOwner() and platform.go (trivial unused helper) - Add license header to audit_windows_test.go
|
All review feedback from the latest round has been addressed in commit d49f93a. Summary of changes: Naming & structure:
Permissions checks:
Mock fixes:
Other:
All 17 CI checks pass (including Windows tests and all integration tests). |
Extract checkACLResult helper in permissions Check() to consistently process ACL findings (report, errors, ACEs) for all checked files. Previously only the system policy file had its ACL result inspected; providers and config.yml silently ignored the ACLReport/ACLErr returned by CheckFilePermissions.
EthanHeilman
left a comment
There was a problem hiding this comment.
This is looks good enough to merge. There are some architectural changes I want to make in this code, but it will go quicker if I make them after windows support is merged.
##### [\`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 #478
Adds a new
opkssh permissionscommand for checking and fixing file permissions/ACLs, and refactors the audit command infrastructure to support cross-platform permission validation.This is the second of three PRs splitting the work from #389 (Windows SSH server support).
Changes
New
permissionscommandAdds
opkssh permissionswith three subcommands:permissions check— Verifies that all opkssh files have correct permissions/ACLs. Reports problems without modifying anything.permissions fix— Repairs permissions/ACLs on opkssh files (requires admin/root).permissions install— Non-interactive permission setup for use by installers. Sets up correct ownership, groups, and permissions for a fresh installation.Permission infrastructure (
policy/files/)PermInfostruct — Centralized definition of required file permissions (mode, owner, group, ACL entries) for each opkssh file type (system policy, home policy, providers, config, binary, log, plugin dir). Platform-specific implementations inperminfo_unix.goandperminfo_windows.go.acl.go,acl_unix.go,acl_windows.go) — Cross-platform interface for verifying file access control lists. Unix implementation checks POSIX modes; Windows implementation checks NTFS DACLs.fileperms_ops.go,fileperms_ops_windows.go) — Cross-platform file ownership and permission manipulation (chown, chmod, ACL modification).permschecker_common.go— Shared permission checker code extracted from the originalpermschecker.go.permschecker_windows.go— Windows-specific permission checker using ACL APIs.sid_windows.go) — Helper for resolving Windows Security Identifiers.Audit command refactors
/etc/opk/paths withpolicy.GetSystemConfigBasePath()andpolicy.SystemDefaultProvidersPath/SystemDefaultPolicyPath.CheckFilePermissions()function used by bothauditandpermissionscommands./etc/passwdparsing toaudit_enum_unix.go; addedaudit_enum_windows.gofor Windows user profile enumeration via registry.audit_windows_profiles.gofor enumerating Windows user profiles, andaudit_windows_test.gofor Windows-specific audit tests.--skip-user-policydefaults totrueon Windows.Policy refactors
paths_unix.go,paths_windows.go):GetSystemConfigBasePath()returns/etc/opkon Unix,%ProgramData%\opkon Windows.policyloader.go: Usesfilepath.Join+GetSystemConfigBasePath()instead of hardcoded paths.multipolicyloader.go: MovedReadWithSudoScriptto platform-specific files (multipolicyloader_unix.gofor sudo,multipolicyloader_windows.goreturning unsupported error).plugins.go: ExportRequiredPolicyDirPerms()for use by the permissions command.Other changes
commands/elevate_unix.go/elevate_windows.go: Platform-specific elevation detection (root check on Unix, admin check on Windows).commands/verify_test_unix.go: Moved Linux-specific verify tests to build-tagged file.commands/add_test.go: Removed test case that relied on platform-specific permission behavior.policy/plugins/plugins_test.go: Removed permission-specific test cases that don't work cross-platform (file mode checks on Windows behave differently).docs/audit.md: Documents the relationship betweenauditandpermissionscommands.Testing
audit_permissions_test.go,audit_windows_test.go,permissions_test.go,permissions_fix_nonwindows_test.go,permissions_fix_windows_test.go,permissions_install_test.go,permissions_mocks_test.go.Related