Conversation
|
💬 Discussion in Slack: #pr-review-cli-318-feat-pam-connection-test-protocol Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
|
| Filename | Overview |
|---|---|
| packages/gateway-v2/test_connection_handler.go | New handler for /v1/test-connection supporting postgres, ssh, and tcp modes; the postgres TLS path has a dangerous default where omitting sslRejectUnauthorized silently enables InsecureSkipVerify |
| packages/gateway-v2/discovery_handler.go | Rename of discoveryTarget/discoveryTargetContextKey to rpcTarget/rpcTargetContextKey and registration of the new /v1/test-connection route; rename-only change with no functional impact |
| go.mod | Adds pgpassfile and pgservicefile as indirect dependencies pulled in by the new pgx/v5 Postgres connection |
| go.sum | Hash entries for the two new indirect dependencies and puddle/v2; no concerns |
Reviews (1): Last reviewed commit: "connection test" | Re-trigger Greptile
| } | ||
|
|
||
| if testErr != nil { | ||
| writeRPCError(w, http.StatusBadGateway, testErr.Error()) |
There was a problem hiding this comment.
Internal error details surfaced verbatim to caller
testErr.Error() is forwarded directly in the 502 response. pgx error strings frequently include the target hostname, port, database name, role name, and the server's diagnostic text (e.g., FATAL: password authentication failed for user "admin" at host:5432). While the endpoint sits behind mTLS, surfacing this verbatim still leaks topology and credential-hint information that could be useful to an attacker who gains access to the response. Consider mapping known error categories (auth failure, unreachable, timeout, TLS) to fixed strings and logging the full error server-side only.
There was a problem hiding this comment.
I think this is fine, since these are provided by the account creator/editor anyways. It would be difficult to rewrite errors for all account and error types
There was a problem hiding this comment.
That's a fair point. Since the errors are returned to the same person who configured the connection in the first place, they already possess all the information being echoed back — hostname, credentials, database name — and seeing the verbatim error is genuinely useful for debugging misconfigured connections. I'll defer to your judgment here.
| case testConnModePostgres: | ||
| testErr = doPostgresConnectionTest(ctx, target.host, target.port, env) | ||
| case testConnModeSSH: | ||
| _, testErr = doSSHExec(target.host, target.port, sshExecEnvelope{ |
There was a problem hiding this comment.
High: SSH password can be sent to an unverified server
This passes connection-test credentials to doSSHExec, which uses ssh.InsecureIgnoreHostKey(). An on-path attacker can impersonate the certificate-bound target and receive the supplied password; require the platform to provide a trusted host key or host CA and verify it before authenticating.
There was a problem hiding this comment.
The way we do test is consistent with discovery. I don't think the attack surface is large especially since gateways are usually within regulated private networks.
PR overviewThis pull request adds a PAM gateway connection-test protocol, including handler logic that attempts target connectivity using supplied credentials such as SSH login details. The PR currently has one open security concern: SSH connection-test credentials may be sent before the SSH server identity is verified. An on-path attacker could impersonate the target during the test and capture the supplied password, creating a clear credential-exposure risk. No issues have been fixed or addressed yet. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
Description 📣
For postgres, ssh, windows.
SSH cert auth and Windows use a basic host reachability check via TLS probe.
Accounts without a password also use a basic TLS probe, but this case is for any future heartbeat functionality on discovered accounts w/o a password.
Type ✨