fix!: validate HostnameSuffix and SSHConfigOptions'#26154
Conversation
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 3 | Last posted: Round 3, 20 findings (2 P2, 8 P3, 8 Nit, 2 Note), APPROVE. Review Finding inventoryFinding inventoryFindings
Contested and acknowledgedCRF-1 (P2, codersdk/deployment.go:777) - ProxyJump missing from SSH option blocklist
CRF-13 (Note, codersdk/deployment.go:776) - Deny-list design fails open against future directives
Round logRound 1Panel. 2 P2, 5 P3, 4 Nit, 2 Note new. 2 dropped. Reviewed against f242ef0..19909be. Round 2Churn guard: PROCEED. 11 addressed, 1 acknowledged, 1 contested. Panel: CRF-1 closed (4/6). 2 P3, 5 Nit new. Reviewed against f242ef0..b2b4cfd. Round 3Churn guard: PROCEED. 7 addressed (a471b89), 0 contested, 0 silent. Netero clean, all fixes verified. Panel: 9/11 no findings, 2 Nits dropped (churn on fixes, contradicted by prior round praise). No new actionable findings. CI green. Reviewed against f242ef0..a471b89. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Solid security-hardening PR. The test coverage is strong (4.3:1 test-to-production ratio), the TDD workflow is genuine, and the defense-in-depth at both server startup and client merge time is the right architecture. The validation consolidation into SSHConfigResponse.Validate() was a good refactoring step.
Severity breakdown: 2 P2, 5 P3, 4 Nit, 2 Note.
The two P2 findings are both blocklist gaps: ProxyJump achieves the same connection-redirection that blocking ProxyCommand was meant to prevent, and PKCS11Provider/SecurityKeyProvider cause the SSH client to load arbitrary shared libraries. Both are straightforward additions to the existing switch block.
Pariston noted: "The newline defense is thorough... the gap is in the key list, not the architecture."
The PR title uses HostNameSuffix (capital N) but the codebase uses HostnameSuffix everywhere. Worth fixing before squash merge.
The deny-list design (vs allow-list) means future dangerous OpenSSH directives will pass validation until someone updates the list. The newline/whitespace check is the more fundamental protection and would catch multi-line injection regardless. Worth noting for future consideration but not blocking.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2. All 11 R1 fixes verified correct. CRF-2 (PKCS11Provider/SecurityKeyProvider) blocklist addition, CRF-3/CRF-4/CRF-7 error message improvements, CRF-5/CRF-6/CRF-9 test improvements, CRF-8/CRF-10 style fixes, CRF-12 non-ASCII whitespace test, CRF-14 DNS label comment migration are all complete.
CRF-1 (ProxyJump): The panel evaluated the author's defense and closed the finding (4/6). ProxyJump does not execute code or load shared libraries. It routes connections through a jump host the deployment admin already controls. Bastion hosts are a legitimate enterprise pattern. The panel concluded that blocking ProxyJump would remove a real deployment capability without meaningfully reducing risk. However, the CRF-7 error message fix introduced a text inconsistency (see CRF-19 below).
Severity breakdown for R2: 2 P3, 5 Nit.
Mafu-san observed: "Multiple R1 findings were fixed beyond the single instance cited. CRF-10 updated both call sites, CRF-12 added NonBreakingSpace to both prefix and suffix tests, CRF-3 applied %q formatting across all validation errors. This is the correct generalization from corrections."
The blocklist classification comment at lines 780-784 is strong engineering documentation. Leorio: "This is exactly what a comment should do: help the person who extends this code a year from now make the right call without reading the PR history."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3. All R2 findings verified fixed. Panel (11 reviewers) found no new actionable issues. CI green.
All 22 findings across 3 rounds are resolved: 18 fixed by author, 1 closed by panel (CRF-1 ProxyJump, 4/6), 1 acknowledged (CRF-13 deny-list design), 2 dropped by orchestrator.
The validation architecture is sound: defense-in-depth at server startup and client merge, centralized SSHConfigResponse.Validate(), layered checks (newline/whitespace prevention + directive blocklist + unicode.IsControl widening). Error messages are consistently formatted, descriptive, and actionable. Test coverage spans injection vectors, blocklist enforcement, case sensitivity, non-ASCII whitespace, and user override behavior at unit, internal, and integration levels (468 test lines for 130 production lines).
Mafu-san: "The commit progression (RED: 398 lines of tests, GREEN: 94 lines, REFACTOR) is genuine TDD. The RED commit touches zero production files. The GREEN commit touches zero test files. This is not staged retroactively."
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Pull request overview
This PR hardens config-ssh deployment configuration by adding server-side and client-side validation for CODER_CONFIGSSH_HOSTNAME_SUFFIX and CODER_SSH_CONFIG_OPTIONS, preventing unsafe values from being served or written into users’ local SSH configs.
Changes:
- Added validation helpers in
codersdkfor workspace hostname prefix/suffix and SSH config options (including disallowed directives). - Validated SSH deployment config during
coder serverstartup (invalid values now fail fast). - Added test coverage for parsing/validation and for CLI rejection behavior (
serverandconfig-ssh).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| codersdk/deployment.go | Adds parsing hardening and validation functions; adds SSHConfigResponse.Validate() used by both server and client. |
| codersdk/deployment_test.go | Adds unit tests for SSH option parsing and validation helpers. |
| cli/server.go | Validates parsed SSH deployment config early in server startup and fails on invalid values. |
| cli/server_test.go | Adds test ensuring invalid SSH deployment flags are rejected at startup. |
| cli/configssh.go | Validates server-provided SSH config before writing any local SSH config updates. |
| cli/configssh_test.go | Adds end-to-end test ensuring config-ssh rejects unsafe server config and does not modify the SSH config file. |
| cli/configssh_internal_test.go | Adds unit tests asserting mergeSSHOptions rejects unsafe server-provided SSH configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
jdomeracki-coder
left a comment
There was a problem hiding this comment.
Approved, additional comments provided on Slack
Docs preview📖 View docs preview for |
Documentation CheckUpdates Needed
All items addressed. The flag description now lists all blocked options (including Automated review via Coder Agents |
mafredri
left a comment
There was a problem hiding this comment.
Caught a few things we could harden, otherwise looks good. 👍🏻
…ct glob characters in hostname suffix
coderdto exit with an error.coder config-sshwill exit with an error if it detects invalid config.Local smoke-testing: ran
develop.sh --env-file <path to an env file containing badness>. Validated that server startup failed as expected.