Resolve security review findings#60
Conversation
| return fmt.Errorf("invalid input path/URL %q: must be on the same host as the configured profile host %q", path, inv.Profile.GetHostname()) | ||
| } | ||
|
|
||
| if resolvedURL.Scheme != "https" { |
There was a problem hiding this comment.
Hostnames are stored exactly as the user entered them (including any scheme), and the client only prepends https:// when no scheme is present also reachable via TFCTL_HOSTNAME. So a profile pointed at an http:// host will now fail every api call, not just absolute http URLs, since relative paths inherit the configured scheme. Is that intended?
There was a problem hiding this comment.
I think this is a bug in how profile hostnames are parsed and created. "hostname" should not contain a scheme.
There was a problem hiding this comment.
Agree this was a real bug. But the TFCTL_HOSTNAME env path still assigns the raw value without validation in loader.go:319-320 it does hostname = envHostname and stores it directly, bypassing ValidateHostname/SetHostname. So TFCTL_HOSTNAME moves forward without any changes. There's already a normalizeHostname helper at loader.go:338 used for token lookups and routing the env value through validation there would help
| return fmt.Errorf("invalid input path/URL %q", path) | ||
| } | ||
|
|
||
| if resolvedURL.Host != apiClient.BaseURL.Host { |
There was a problem hiding this comment.
The same-host check on this line and and https check on 264 live inside the NewCmdAPI closure, so they can't be unit-tested via an Opts value. RunAPI line 379 already receives opts.Client.BaseURL and opts.URL so moving these two checks into RunAPI aligns with our conventions and lets us add a positive-acceptance test (currently only rejection paths are reachable).
There was a problem hiding this comment.
I don't want to move these into RunOpts because then I'd have to extend opts to contain profile information. I'm only worried about rejection paths in tests because those are the only possible branching conditions. I can do a normal Run test with valid arguments and get normal results. Long story short, I like the code
|
|
||
| if opts.DryRun { | ||
| cs := opts.IO.ColorScheme() | ||
| fmt.Fprintf(opts.IO.Err(), "%s would set profile property %q to %q\n", cs.DryRunLabel(), opts.Property, opts.Value) |
There was a problem hiding this comment.
nit: --dry-run prints the raw opts.Value, but the value that actually gets stored is normalized. The dry-run output should show the normalized hostname so it matches what a real run would write.
| // SetHostname sets the profile's hostname after validating it. The hostname should be a hostname | ||
| // with an optional port, and should not include a scheme. If the hostname includes a scheme, the | ||
| // scheme will be stripped. | ||
| func (p *Profile) SetHostname(hostname string) error { |
There was a problem hiding this comment.
nit: There is no nil-receiver guard like the one SetDefaultOrganization has
| // ValidateHostname validates that the provided hostname is a valid hostname with an optional port, | ||
| // and does not include a scheme. If the hostname includes a scheme, the scheme is stripped before | ||
| // validation. | ||
| func (p *Profile) ValidateHostname(hostname string) (string, error) { |
There was a problem hiding this comment.
doesn't use p could be a package level function
| The `tfctl` CLI provides high-level commands for common workflows, such as managing runs, variables, and workspaces, and direct API access for advanced automation. It supports multiple configuration profiles, allowing you to switch between different HCP Terraform organizations and Terraform Enterprise instances. It also integrates with AI coding agents to facilitate agent-assisted management of Terraform workflows. | ||
|
|
||
|  | ||
|  |
There was a problem hiding this comment.
should we delete the hero.png file
brandonc
force-pushed
the
TF-38447-resolve-security-review-findings
branch
from
828d99e to
4c10c7a
Compare
| } | ||
|
|
||
| hostname := DefaultHostname | ||
| if envHostname, ok := os.LookupEnv(envVarHostname); ok && envHostname != "" { |
There was a problem hiding this comment.
TFCTL_HOSTNAME is still stored raw here it skips NormalizeHostname, so the scheme/path-stripping and IDNA work done for create/set doesn't apply to the env override.
if envHostname, ok := os.LookupEnv(envVarHostname); ok && envHostname != "" {
if normalized, err := NormalizeHostname(envHostname); err == nil {
hostname = normalized
}
}
|
|
||
| // NormalizeHostname validates and normalizes the given hostname by stripping any extra URL data, | ||
| // like paths. It also converts domain names to their idna ASCII form. | ||
| func NormalizeHostname(hostname string) (string, error) { |
There was a problem hiding this comment.
NormalizeHostname("") returns ("", nil) because idna.Lookup.ToASCII("") succeeds and returns before the validHostnamePattern check (line 296). The old ValidateHostname rejected empty. This is reachable via tfctl profile set hostname "", which now passes validation and since a hostname change unconditionally clears credentials silently wipes the token and default_organization. I would suggest rejecting empty explicitly, like at the top:
if strings.TrimSpace(hostname) == "" {
return "", fmt.Errorf("hostname cannot be empty")
}
There was a problem hiding this comment.
I think this is what we want. Empty hostname restores the default hostname.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| logger.Debug("Setting token from terraform credentials file", "hostname", c.Hostname) |
There was a problem hiding this comment.
This logs "Setting token from terraform credentials file" (and assigns tokenFromEnv) whenever the profile has no token, even when tokenFromCredentials returned "" (no match). Steps 1 and 3 only log inside their if … != "" guards this one should too:
if credsToken != "" {
logger.Debug("Setting token from terraform credentials file", "hostname", c.Hostname)
c.tokenFromEnv = credsToken
}
| if c.Token == "" { | ||
| if envToken := os.Getenv(legacyTokenEnvVar(c.Hostname)); envToken != "" { | ||
| // 2. Check for a token in the terraform credentials file that matches the hostname of the profile | ||
| if c.GetToken() == "" { |
There was a problem hiding this comment.
TFCTL_TOKEN now takes precedence over the Terraform credentials file (previously the creds file was checked first) is this intended?
There was a problem hiding this comment.
Yeah, i think this one is the most relevant
| // profile name. | ||
| ErrInvalidProfileName = errors.New("profile name may only include a-z, A-Z, 0-9, or '_', must start with a letter, and can be no longer than 64 characters") | ||
|
|
||
| validHostnamePattern = regexp.MustCompile(`^[a-zA-Z0-9.-]+(:\d+)?$`) |
There was a problem hiding this comment.
nit: validHostnamePattern rejects IPv6 literals and after the refactor those inputs surface the raw url.Parse error ("first path segment in URL cannot contain colon") rather than the "must be a valid hostname" message
2 participants
Description
apiargument to subvert token stealingPR Checklist
--json— Force machine readable output to stdout. Does not apply to stderr.--markdown— Force markdown output to stdout. Does not apply to stderr.--dry-run— Don't make any actual writes or other mutations. Describe what would have changed to stderr.--quiet— Don't render output to stdout.make gen/screenshotif the root command output changes.Autocompletefield to positional arguments and flags to assist shell autocomplete.changie newto prepare a new changelog entry for the next set of release notes.