fix(aws): harden AWS SSO importer and provisioner against hostile inputs

Address findings from a pre-PR security review. Each fix is local to the
AWS SSO net-new code introduced in the previous commit; pre-existing SDK
bugs surfaced by the same review are deferred to a follow-up PR.

Importer (plugins/aws/sso_importer.go):
- Validate sso_start_url (https + non-empty host)
- Regex-check sso_account_id (12 digits) and sso_region
- Reject NUL bytes mid-value
- Refuse non-regular, symlinked, or non-owned AWS_CONFIG_FILE overrides
- Match SDK ~/ prefix convention (bare ~root no longer silently joined)
- Use ini.LoadSources directly with strict botocore-parity options:
  KeyValueDelimiters="=", IgnoreContinuation=true, Loose=true

Provisioner (plugins/aws/sso_provisioner.go):
- assertSSOTokenCacheSafe rejects symlinks, non-regular files, group/world
  readable modes, and files not owned by the current uid before passing
  the path to ssocreds.New
- translateSSORetrieveError whitelists known smithy codes (Unauthorized,
  Forbidden, ResourceNotFound, TooManyRequests); unknown codes get a
  generic plugin-controlled message so server-controlled error text
  doesn't reach user-visible output
- Wrap sso:GetRoleCredentials in context.WithTimeout(30s)

SDK validator (sdk/schema/credential_type.go):
- Replace the previous severity downgrade with an opt-in
  AllowsExternalSecretCache flag on CredentialType. The "must have at
  least one secret field" check is restored to Error severity globally;
  only the SSO Profile (whose bearer token lives in the AWS SDK's
  external cache) opts out

Tests (plugins/aws/sso_*_test.go):
- Hostile-input cases for the importer (non-HTTPS, file:// scheme, short
  account ID, malformed region, NUL byte, malformed section header,
  duplicate-section last-wins)
- Direct unit tests for assertSSOTokenCacheSafe and
  validateExternalConfigPath covering symlink, world-readable,
  directory, non-existent
- Smithy-error translation table with a token-leak guard that asserts no
  JSON-key-shaped or JWT-shaped fragment from a hostile server message
  ever appears in the translated user-visible error

Deferred to follow-up PR (pre-existing code, out of scope here):
- F-2: plugins/registry.go:50-60 GetCredentialType ignores credentialName
- F-6: sdk/schema/plugin.go:116-134 MarshalJSON truncates to Credentials[0]
- F-7: plugins/aws/cli_provisioner.go:28-53 strip-by-value mis-routing
- F-9: sdk/importer/helpers.go:41-53 SanitizeNameHint byte-truncation
- F-15: plugins/aws/sts_provisioner.go:399-405 log.SetOutput global state

Author: rjwhitworth

go.mod

@@ -34,7 +34,7 @@ require (
 	github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.9.23 // indirect
 	github.com/aws/aws-sdk-go-v2/service/sso v1.12.4
 	github.com/aws/aws-sdk-go-v2/service/ssooidc v1.14.4 // indirect
-	github.com/aws/smithy-go v1.13.5 // indirect
+	github.com/aws/smithy-go v1.13.5
 	github.com/danieljoos/wincred v1.1.2 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/dvsekhvalnov/jose2go v1.6.0 // indirect

plugins/aws/aws.go

@@ -15,16 +15,23 @@ func AWSCLI() schema.Executable {
 		NeedsAuth: needsauth.IfAll(
 			needsauth.NotForHelpOrVersion(),
 			needsauth.NotWithoutArgs(),
-			// `aws sso login` and `aws sso logout` write/erase the very token
-			// the SSO Profile provisioner reads from disk; provisioning before
-			// they run creates a chicken-and-egg failure.
+			// Each entry below intentionally bypasses provisioning. Subcommands not listed here
+			// (e.g. `aws configure import`, `aws configure export-credentials`) still flow through
+			// the provisioner because they exchange or mutate credentials and benefit from the
+			// 1Password-managed surface.
+			//
+			// `aws sso login`/`logout` write/erase the very token the SSO Profile provisioner reads
+			// from disk; provisioning before they run creates a chicken-and-egg failure.
 			needsauth.NotWhenContainsArgs("sso", "login"),
 			needsauth.NotWhenContainsArgs("sso", "logout"),
-			// `aws configure sso` and `aws configure sso-session` set up the
-			// SSO configuration in ~/.aws/config; no provisioned credentials needed.
+			// `aws configure sso` and `aws configure sso-session` set up SSO configuration in
+			// ~/.aws/config; they touch no AWS APIs and need no provisioned credentials.
 			needsauth.NotWhenContainsArgs("configure", "sso"),
 			needsauth.NotWhenContainsArgs("configure", "sso-session"),
-			// Read-only / diagnostic configure subcommands don't make API calls.
+			// `aws configure list` and `list-profiles` are read-only diagnostic queries against
+			// the local config; `get` and `set` mutate ~/.aws/config or ~/.aws/credentials but
+			// make no remote API call. Provisioning would be a no-op at best and a confusing
+			// "missing credentials" error at worst when the user is just inspecting state.
 			needsauth.NotWhenContainsArgs("configure", "list"),
 			needsauth.NotWhenContainsArgs("configure", "list-profiles"),
 			needsauth.NotWhenContainsArgs("configure", "get"),

plugins/aws/sso_importer.go

@@ -3,8 +3,11 @@ package aws
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"os"
+	"regexp"
 	"strings"
+	"syscall"
 
 	"gopkg.in/ini.v1"
 
@@ -18,21 +21,45 @@ const (
 	ssoSessionSectionPrefix = "sso-session "
 )
 
+// AWS account IDs are exactly 12 decimal digits; AWS regions are lowercase letters with one or two
+// dashes separating segments and a trailing digit (e.g. `us-east-1`, `ap-southeast-2`).
+var (
+	ssoAccountIDRE = regexp.MustCompile(`^[0-9]{12}$`)
+	ssoRegionRE    = regexp.MustCompile(`^[a-z]{2}-[a-z]+-[0-9]+$`)
+)
+
 // TrySSOConfigFile looks for AWS IAM Identity Center profiles in ~/.aws/config.
 // It supports both the legacy form (sso_start_url on the profile) and the
 // consolidated form (sso_session = NAME referencing an [sso-session NAME] section).
 func TrySSOConfigFile() sdk.Importer {
 	return func(ctx context.Context, in sdk.ImportInput, out *sdk.ImportOutput) {
 		sourcePath := os.Getenv("AWS_CONFIG_FILE")
-		if sourcePath == "" {
+		envOverride := sourcePath != ""
+		if !envOverride {
 			sourcePath = "~/.aws/config"
 		}
 
-		configPath := sourcePath
-		if strings.HasPrefix(configPath, "~") {
-			configPath = in.FromHomeDir(strings.TrimPrefix(configPath, "~"))
-		} else {
-			configPath = in.FromRootDir(configPath)
+		// Match the SDK convention (sdk/importer/file_importer.go): only `~/` (with slash) is
+		// expanded against the home directory. A bare `~root/...` form would otherwise be silently
+		// joined to the *current* user's home, which is misleading at best and security-relevant if
+		// an attacker can pre-place a file there.
+		var configPath string
+		switch {
+		case strings.HasPrefix(sourcePath, "~/"):
+			configPath = in.FromHomeDir(strings.TrimPrefix(sourcePath, "~/"))
+		default:
+			configPath = in.FromRootDir(sourcePath)
+		}
+
+		// When `AWS_CONFIG_FILE` is honoured, refuse anything that is not a regular file owned by
+		// the current user. Defends against a malicious `.envrc` (direnv, asdf, mise) pinning the
+		// importer to a hostile config file when `op` is run from that directory.
+		if envOverride {
+			if err := validateExternalConfigPath(configPath); err != nil {
+				attempt := out.NewAttempt(importer.SourceFile(sourcePath))
+				attempt.AddError(err)
+				return
+			}
 		}
 
 		contents, err := os.ReadFile(configPath)
@@ -51,9 +78,23 @@ func TrySSOConfigFile() sdk.Importer {
 
 		attempt := out.NewAttempt(importer.SourceFile(sourcePath))
 
-		configFile, err := importer.FileContents(contents).ToINI()
+		// Strict botocore-parity loader: `=` is the only key/value delimiter (botocore rejects `:`),
+		// line continuation is disabled (botocore reads each `\` literally), and `Loose: true` lets
+		// the loader produce a partial result instead of bricking the entire import on a single
+		// malformed section. AllowShadows defaults to false, which matches botocore last-wins
+		// semantics for duplicate `[profile X]` sections; we surface no diagnostic to keep parity.
+		configFile, err := ini.LoadSources(ini.LoadOptions{
+			KeyValueDelimiters: "=",
+			IgnoreContinuation: true,
+			Loose:              true,
+		}, contents)
 		if err != nil {
 			attempt.AddError(err)
+			// Fall through; with Loose: true the loader returns a partial *ini.File on most parse
+			// errors and only short-circuits on truly catastrophic ones. Either way, valid sections
+			// in the result still produce candidates.
+		}
+		if configFile == nil {
 			return
 		}
 
@@ -82,6 +123,32 @@ func TrySSOConfigFile() sdk.Importer {
 	}
 }
 
+// validateExternalConfigPath enforces the safety properties for an `AWS_CONFIG_FILE` override:
+// the path must resolve to a regular file owned by the current user. Anything else is rejected.
+func validateExternalConfigPath(path string) error {
+	fi, err := os.Lstat(path)
+	if err != nil {
+		// If the path does not exist, downstream `os.ReadFile` will report it; treat it as a
+		// non-error here so we do not double-report.
+		if os.IsNotExist(err) {
+			return nil
+		}
+		return fmt.Errorf("AWS_CONFIG_FILE %q: %w", path, err)
+	}
+	if fi.Mode()&os.ModeSymlink != 0 {
+		return fmt.Errorf("AWS_CONFIG_FILE %q is a symlink; refusing to follow", path)
+	}
+	if !fi.Mode().IsRegular() {
+		return fmt.Errorf("AWS_CONFIG_FILE %q is not a regular file", path)
+	}
+	if st, ok := fi.Sys().(*syscall.Stat_t); ok {
+		if st.Uid != uint32(os.Geteuid()) {
+			return fmt.Errorf("AWS_CONFIG_FILE %q is not owned by the current user", path)
+		}
+	}
+	return nil
+}
+
 // profileNameFromSection returns the bare profile name for a [profile NAME] or
 // [default] section, and false for any other section type.
 func profileNameFromSection(sectionName string) (string, bool) {
@@ -106,9 +173,10 @@ func collectSSOSessions(configFile *ini.File) map[string]*ini.Section {
 	return sessions
 }
 
-// buildSSOFields returns the candidate fields for a profile section, or (nil, nil)
-// if the profile is not SSO-bearing or is missing required keys. A non-nil error
-// indicates a malformed reference (e.g. unknown sso_session) and should be reported.
+// buildSSOFields returns the candidate fields for a profile section, or (nil, nil) if the profile
+// is not SSO-bearing or is missing required keys. A non-nil error indicates a malformed reference
+// or an invalid value (bad URL, malformed account ID, etc.) and should be reported via
+// `attempt.AddError`. Errors are scoped to a single profile so other valid profiles still import.
 func buildSSOFields(profileName string, section *ini.Section, ssoSessions map[string]*ini.Section) (map[sdk.FieldName]string, error) {
 	hasSSOSession := keyHasValue(section, "sso_session")
 	hasLegacyStartURL := keyHasValue(section, "sso_start_url")
@@ -120,6 +188,9 @@ func buildSSOFields(profileName string, section *ini.Section, ssoSessions map[st
 
 	if hasSSOSession {
 		sessionName := section.Key("sso_session").Value()
+		if containsNUL(sessionName) {
+			return nil, fmt.Errorf("profile %q sso_session value contains a NUL byte", profileName)
+		}
 		sessionSection, ok := ssoSessions[sessionName]
 		if !ok {
 			return nil, fmt.Errorf("profile %q references unknown sso-session %q", profileName, sessionName)
@@ -152,9 +223,46 @@ func buildSSOFields(profileName string, section *ini.Section, ssoSessions map[st
 		return nil, nil
 	}
 
+	for _, v := range fields {
+		if containsNUL(v) {
+			return nil, fmt.Errorf("profile %q contains a NUL byte in one of its SSO fields", profileName)
+		}
+	}
+
+	if err := validateSSOStartURL(fields[fieldname.SSOStartURL]); err != nil {
+		return nil, fmt.Errorf("profile %q: %w", profileName, err)
+	}
+	if !ssoAccountIDRE.MatchString(fields[fieldname.SSOAccountID]) {
+		return nil, fmt.Errorf("profile %q: sso_account_id %q is not a 12-digit AWS account ID", profileName, fields[fieldname.SSOAccountID])
+	}
+	if !ssoRegionRE.MatchString(fields[fieldname.SSORegion]) {
+		return nil, fmt.Errorf("profile %q: sso_region %q is not a valid AWS region", profileName, fields[fieldname.SSORegion])
+	}
+
 	return fields, nil
 }
 
+// validateSSOStartURL enforces an HTTPS scheme and a non-empty host. We do not pin the host to
+// `*.awsapps.com`: AWS supports custom SSO start URLs (e.g. `https://signin.aws.amazon.com/...`),
+// and an over-tight allowlist would reject legitimate enterprise configurations.
+func validateSSOStartURL(s string) error {
+	u, err := url.Parse(s)
+	if err != nil {
+		return fmt.Errorf("sso_start_url %q is not a valid URL: %w", s, err)
+	}
+	if u.Scheme != "https" {
+		return fmt.Errorf("sso_start_url %q must use https://", s)
+	}
+	if u.Host == "" {
+		return fmt.Errorf("sso_start_url %q has no host component", s)
+	}
+	return nil
+}
+
+func containsNUL(s string) bool {
+	return strings.IndexByte(s, 0) >= 0
+}
+
 func keyHasValue(section *ini.Section, key string) bool {
 	return section.HasKey(key) && section.Key(key).Value() != ""
 }