feat(pam): add --target flag to select a host on pam access#286
Conversation
|
💬 Discussion in Slack: #pr-review-cli-286-feat-pam-add-target-flag-to-select-a-host-on-pam-access Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
|
| Filename | Overview |
|---|---|
| packages/cmd/pam.go | Adds --target flag and threads it through to StartPAMAccess; value is passed to the API unvalidated, which is a potential SSRF vector |
| packages/pam/local/access.go | Renames AccountTypeActiveDirectory to AccountTypeWindowsAd, enables Windows AD via RDP proxy; rename is a breaking change if not coordinated with server |
| packages/api/model.go | Adds TargetHost field to PAMAccessRequest; straightforward model addition with omitempty |
Reviews (1): Last reviewed commit: "target flag + domain acc tweaks" | Re-trigger Greptile
| targetHost, err := cmd.Flags().GetString("target") | ||
| if err != nil { | ||
| util.HandleError(err, "Unable to parse target flag") | ||
| } |
There was a problem hiding this comment.
Unvalidated user-supplied host passed to API (SSRF)
targetHost is accepted from the command line and forwarded verbatim to the API as targetHost in the PAM access request. If the server uses this value to initiate a backend connection without validating it against a per-account allowlist, a user could supply an arbitrary internal address (e.g., 169.254.169.254, 10.0.0.1) to pivot through the gateway into network segments they would not otherwise reach. Client-side format validation (e.g., rejecting bare IPs, enforcing a valid FQDN pattern) would reduce the attack surface, but the authoritative guard must live on the server side.
Context Used: Flag SSRF risks (source)
| AccountTypeKubernetes = "kubernetes" | ||
| AccountTypeAwsIam = "aws-iam" | ||
| AccountTypeWindows = "windows" | ||
| AccountTypeWindowsAd = "windows-ad" |
There was a problem hiding this comment.
Renamed enum value may break existing sessions
AccountTypeActiveDirectory = "active-directory" has been replaced by AccountTypeWindowsAd = "windows-ad". The AccountType field is populated from the API response, so if the server continues to return "active-directory" for any existing AD accounts (e.g., before a coordinated server-side deploy), the switch statement will fall through to the default branch and print "Unsupported account type: active-directory" instead of starting the RDP proxy. Make sure the server deployment that introduces "windows-ad" is released atomically with (or before) this CLI change to avoid a regression window.
Description 📣
Add --target flag to select a host on pam access
Type ✨
Bug fix
New feature
Improvement
Breaking change
Documentation
I have read the contributing guide, agreed and acknowledged the code of conduct. 📝