windows: Add process-level verification to named pipe#114
Conversation
The named pipe previously relied only on SDDL for access control (Administrators, SYSTEM, crc-users), allowing any process under those identities to access the API This adds client process verification to ensure only crc.exe can access the pipe, based on the name (crc.exe) and the full path under C:\Program Files in case of release builds Since writing to C:\Program File requires admin privileges, full the path check cannot be satisfied without already having admin privileges Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
WalkthroughThe change adds a release-build flag propagated through Go linker flags and introduces Windows named-pipe peer verification. The listener resolves the client executable, requires ChangesWindows peer verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/admin-helper/peercheck_windows.go (1)
29-33: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffBypass risk via handle duplication and PID reuse.
Relying on
GetNamedPipeClientProcessIdis subject to a known Windows IPC limitation. A malicious client can open the pipe, duplicate the handle to a child process, and then terminate.GetNamedPipeClientProcessIdwill still return the original PID (the creator of the handle). If the attacker can force the OS to reuse that original PID for a newly spawned legitimatecrc.exeinstance,OpenProcesswill validate the legitimate process, while the server unknowingly communicates with the malicious child holding the duplicated handle.Consider treating this path check primarily as a defense-in-depth measure. For robust security boundaries across named pipes, the client should ideally prove its identity over the pipe stream (e.g., passing a cryptographic token).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/admin-helper/peercheck_windows.go` around lines 29 - 33, Update getClientExePath to treat GetNamedPipeClientProcessId and subsequent executable-path validation only as defense-in-depth, not as authoritative client authentication. Preserve the existing checks, and add an authenticated identity proof over the named-pipe stream (such as a cryptographic token) before trusting the client; ensure callers reject connections when proof validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/admin-helper/peercheck_windows.go`:
- Around line 84-87: Update the unauthorized-process warning in the peer-check
logic around the strings.EqualFold check to remove the hardcoded “(dev build)”
qualifier, while preserving the existing message context and structured
process/expected fields.
- Around line 17-18: Update the allowedProcessPath initialization in the Windows
peer-check logic to avoid hardcoding the C: drive; derive the expected crc.exe
location dynamically from the installation context, preferably using the
executable location or the ProgramFiles environment variable while preserving
the existing allowedProcessName validation.
In `@pkg/constants/build.go`:
- Around line 3-7: The ReleaseBuild documentation in the constants file promises
Authenticode publisher validation, but peer verification currently only enforces
the executable path. Either implement Authenticode signature and
expected-publisher validation in the Windows peer-check flow, or revise the
ReleaseBuild comment to describe path-only verification; keep the documented
behavior aligned with the actual implementation.
---
Nitpick comments:
In `@cmd/admin-helper/peercheck_windows.go`:
- Around line 29-33: Update getClientExePath to treat
GetNamedPipeClientProcessId and subsequent executable-path validation only as
defense-in-depth, not as authoritative client authentication. Preserve the
existing checks, and add an authenticated identity proof over the named-pipe
stream (such as a cryptographic token) before trusting the client; ensure
callers reject connections when proof validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 198d6232-7ddd-4df8-b8c7-5cfc2c4c7c96
📒 Files selected for processing (4)
Makefilecmd/admin-helper/listen_windows.gocmd/admin-helper/peercheck_windows.gopkg/constants/build.go
| allowedProcessName = "crc.exe" | ||
| allowedProcessPath = `C:\Program Files\Red Hat OpenShift Local\crc.exe` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid hardcoding the system drive in the expected installation path.
Hardcoding C:\Program Files\... will erroneously reject valid connections from users who customize their installation directory (e.g., installing to a D: drive) or who have a different system drive letter.
Consider resolving the expected path dynamically. If crc-admin-helper.exe and crc.exe are guaranteed to reside in known relative locations, resolve the path relative to os.Executable(). At a minimum, consider building the path using the ProgramFiles environment variable to support arbitrary system drives.
💡 Example using the environment variable
const (
allowedProcessName = "crc.exe"
- allowedProcessPath = `C:\Program Files\Red Hat OpenShift Local\crc.exe`
)
+
+var allowedProcessPath = filepath.Join(os.Getenv("ProgramFiles"), "Red Hat OpenShift Local", allowedProcessName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allowedProcessName = "crc.exe" | |
| allowedProcessPath = `C:\Program Files\Red Hat OpenShift Local\crc.exe` | |
| allowedProcessName = "crc.exe" | |
| var allowedProcessPath = filepath.Join(os.Getenv("ProgramFiles"), "Red Hat OpenShift Local", allowedProcessName) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/admin-helper/peercheck_windows.go` around lines 17 - 18, Update the
allowedProcessPath initialization in the Windows peer-check logic to avoid
hardcoding the C: drive; derive the expected crc.exe location dynamically from
the installation context, preferably using the executable location or the
ProgramFiles environment variable while preserving the existing
allowedProcessName validation.
| if !strings.EqualFold(exeName, allowedProcessName) { | ||
| vl.logger.Warn("peer check: rejecting connection from unauthorized process (dev build)", | ||
| "process", exePath, | ||
| "expected", allowedProcessName) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove hardcoded (dev build) from the log message.
This code block evaluates in both dev and release builds. If a process with an unauthorized name connects during a release build, the log will incorrectly state it is a dev build, which can cause confusion during debugging.
♻️ Proposed fix
- vl.logger.Warn("peer check: rejecting connection from unauthorized process (dev build)",
+ vl.logger.Warn("peer check: rejecting connection from unauthorized process",
"process", exePath,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !strings.EqualFold(exeName, allowedProcessName) { | |
| vl.logger.Warn("peer check: rejecting connection from unauthorized process (dev build)", | |
| "process", exePath, | |
| "expected", allowedProcessName) | |
| if !strings.EqualFold(exeName, allowedProcessName) { | |
| vl.logger.Warn("peer check: rejecting connection from unauthorized process", | |
| "process", exePath, | |
| "expected", allowedProcessName) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/admin-helper/peercheck_windows.go` around lines 84 - 87, Update the
unauthorized-process warning in the peer-check logic around the
strings.EqualFold check to remove the hardcoded “(dev build)” qualifier, while
preserving the existing message context and structured process/expected fields.
| // ReleaseBuild is set to "true" via -ldflags in release builds. | ||
| // When "true", peer process verification on Windows requires valid | ||
| // Authenticode signatures from the expected publisher and an exact | ||
| // path match. | ||
| var ReleaseBuild = "false" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Missing Authenticode signature check or misleading comment.
The comment indicates that peer process verification requires "valid Authenticode signatures from the expected publisher", but the implementation in peercheck_windows.go only verifies the executable path.
If the Authenticode check was intentionally omitted, please update the comment to match the implementation. If it is missing, it should be implemented to ensure robust verification, as path checks alone do not cryptographically guarantee the binary's integrity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/constants/build.go` around lines 3 - 7, The ReleaseBuild documentation in
the constants file promises Authenticode publisher validation, but peer
verification currently only enforces the executable path. Either implement
Authenticode signature and expected-publisher validation in the Windows
peer-check flow, or revise the ReleaseBuild comment to describe path-only
verification; keep the documented behavior aligned with the actual
implementation.
2 participants
The named pipe previously relied only on SDDL for access control (Administrators, SYSTEM, crc-users), allowing any process under those identities to access the API
This adds client process verification to ensure only crc.exe can access the pipe, based on the name (crc.exe) and the full path under C:\Program Files in case of release builds
Since writing to C:\Program File requires admin privileges, full the path check cannot be satisfied without already having admin privileges
Summary by CodeRabbit
Security
Bug Fixes