internal/auth/identities/aws/assume_role.go (1)

165-175: Honor interface: return via.provider only; don’t fall back to via.identity.

The Identity contract says non-user identities return via.provider. Returning via.identity violates it and can break consumers.

 func (i *assumeRoleIdentity) GetProviderName() (string, error) {
 	if i.config.Via != nil && i.config.Via.Provider != "" {
 		return i.config.Via.Provider, nil
 	}
-	if i.config.Via != nil && i.config.Via.Identity != "" {
-		// This assume role identity chains through another identity.
-		// For caching purposes, we'll use the chained identity name.
-		return i.config.Via.Identity, nil
-	}
-	return "", fmt.Errorf("%w: assume role identity %q has no valid via configuration", errUtils.ErrInvalidIdentityConfig, i.name)
+	return "", fmt.Errorf("%w: assume role identity %q has no valid via.provider configuration", errUtils.ErrInvalidIdentityConfig, i.name)
 }

tests/fixtures/scenarios/atmos-auth/atmos.yaml (1)

10-21: Ensure schemas validate new auth keys (providers/identities).

We need JSON Schemas to expose/validate auth.providers and auth.identities so this fixture passes validation. Prior reviews flagged this already.

Run to confirm coverage and identify files to update:

#!/bin/bash
set -euo pipefail

echo "Search Go types (expect Auth/Provider/Identity present):"
rg -nP 'type\s+(Auth|AuthConfig|Provider|Identity)\b' -g '!**/vendor/**' || true

echo
echo "Check JSON Schemas expose 'auth' at root and define providers/identities:"
fd -H -t f '1.0.json' pkg/datafetcher/schema | while read -r f; do
  echo "== $f"
  rg -nC2 '"auth"' "$f" || true
  rg -nC2 '"providers"' "$f" || true
  rg -nC2 '"identities"' "$f" || true
done

Happy to draft the schema additions (root auth property + definitions.auth/providers/identities).

internal/auth/providers/aws/saml.go (3)

264-277: Use case-insensitive detection and return “GoogleApps” for Google Workspace.

saml2aws expects Provider="GoogleApps" for Google Workspace SAML; also normalize URL checks.

-	// Auto-detect provider type based on URL
-	if strings.Contains(p.url, "accounts.google.com") {
-		return "Browser" // Use Browser for Google Apps SAML for better compatibility
-	}
-	if strings.Contains(p.url, "okta.com") {
+	// Auto-detect provider type based on URL.
+	u := strings.ToLower(p.url)
+	if strings.Contains(u, "accounts.google.com") {
+		return "GoogleApps"
+	}
+	if strings.Contains(u, "okta.com") {
 		return "Okta"
 	}
-	if strings.Contains(p.url, "adfs") {
+	if strings.Contains(u, "adfs") {
 		return "ADFS"
 	}

Please confirm the current provider strings in versent/saml2aws v2:

For versent/saml2aws v2, what are the valid values for cfg.IDPAccount.Provider and the correct value for Google Workspace SAML?

---

106-109: Ensure saml2aws config dir exists before client/auth.

Create ~/.saml2aws proactively to prevent first-run failures. This was raised previously.

 	// Create config and client + login details
 	samlConfig := p.createSAMLConfig()
+	if err := ensureSAML2AWSConfigDir(); err != nil {
+		return nil, fmt.Errorf("%w: failed to ensure saml2aws config dir: %v", errUtils.ErrInvalidProviderConfig, err)
+	}
 	loginDetails := p.createLoginDetails()

Add this helper in the same file:

// add to imports:
// "path/filepath"

func ensureSAML2AWSConfigDir() error {
	home, err := os.UserHomeDir()
	if err != nil {
		return fmt.Errorf("%w: get user home directory: %v", errUtils.ErrInvalidProviderConfig, err)
	}
	dir := filepath.Join(home, ".saml2aws")
	if err := os.MkdirAll(dir, 0o700); err != nil {
		return fmt.Errorf("%w: create saml2aws config directory: %v", errUtils.ErrInvalidProviderConfig, err)
	}
	return nil
}

---

241-253: Prevent nil deref on STS response.

STS can return a nil result or nil Credentials on error paths. Guard before dereferencing.

 result, err := stsClient.AssumeRoleWithSAML(ctx, input)
 if err != nil {
 	return nil, fmt.Errorf("%w: failed to assume role with SAML: %v", errUtils.ErrAuthenticationFailed, err)
 }
+if result == nil || result.Credentials == nil {
+	return nil, fmt.Errorf("%w: STS returned empty credentials", errUtils.ErrAuthenticationFailed)
+}
 
 // Convert to AWSCredentials
 creds := &types.AWSCredentials{
-	AccessKeyID:     aws.ToString(result.Credentials.AccessKeyId),
-	SecretAccessKey: aws.ToString(result.Credentials.SecretAccessKey),
-	SessionToken:    aws.ToString(result.Credentials.SessionToken),
+	AccessKeyID:     aws.ToString(result.Credentials.AccessKeyId),
+	SecretAccessKey: aws.ToString(result.Credentials.SecretAccessKey),
+	SessionToken:    aws.ToString(result.Credentials.SessionToken),
 	Region:          p.region,
 	Expiration:      result.Credentials.Expiration.Format(time.RFC3339),
 }

internal/auth/identities/aws/assume_role.go (3)

73-79: Validate and bound DurationSeconds to STS limits (900–43200s).

Prevents negatives or out-of-range values; keeps server-side errors out of the happy path.

 	if durationStr, ok := i.config.Principal["duration"].(string); ok && durationStr != "" {
-		if duration, err := time.ParseDuration(durationStr); err == nil {
-			input.DurationSeconds = aws.Int32(int32(duration.Seconds()))
-		} else {
-			log.Warn("Invalid duration specified for assume role", "duration", durationStr)
-		}
+		if duration, err := time.ParseDuration(durationStr); err == nil {
+			secs := int32(duration.Seconds())
+			if secs < 0 {
+				log.Warn("Negative duration specified for assume role; ignoring.", "duration", durationStr)
+			} else {
+				if secs < 900 {
+					secs = 900
+				} else if secs > 43200 {
+					secs = 43200
+				}
+				input.DurationSeconds = aws.Int32(secs)
+			}
+		} else {
+			log.Warn("Invalid duration specified for assume role.", "duration", durationStr)
+		}
 	}

---

65-69: Truncate session name to 64 chars to satisfy STS constraints.

AWS STS enforces a 64‑char max for RoleSessionName.

-	sessionName := fmt.Sprintf("atmos-%s-%d", i.name, time.Now().Unix())
+	sessionName := fmt.Sprintf("atmos-%s-%d", i.name, time.Now().Unix())
+	if len(sessionName) > 64 {
+		sessionName = sessionName[:64]
+	}

---

27-42: DRY region fallback logic.

With the Authenticate change pinning i.region, newSTSClient doesn’t need its own fallback; it can assume i.region is set. Optional cleanup.

 func (i *assumeRoleIdentity) newSTSClient(ctx context.Context, awsBase *types.AWSCredentials) (*sts.Client, error) {
-	region := i.region
-	if region == "" {
-		region = awsBase.Region
-	}
-	if region == "" {
-		region = "us-east-1"
-	}
-	cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(region))
+	cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(i.region))
 	if err != nil {
 		return nil, fmt.Errorf("%w: failed to load AWS config: %v", errUtils.ErrInvalidIdentityConfig, err)
 	}

tests/fixtures/scenarios/atmos-auth/atmos.yaml (1)

12-14: Double-check GitHub OIDC provider region support
I didn’t find any code handling a region field for the github/oidc provider; verify whether it’s used—remove it if unused or document its purpose.

internal/auth/providers/aws/saml.go (3)

98-101: Do not log IdP URLs at info level.

Reduce exposure; URL can contain tenant details. Move to debug and drop URL.

-	log.Info("Starting SAML authentication", "provider", p.name, "url", p.url)
+	log.Debug("Starting SAML authentication.", "provider", p.name)

---

12-12: Avoid import alias shadowing the local package name.

Rename the AWS SDK alias to reduce cognitive load.

-import (
-	aws "github.com/aws/aws-sdk-go-v2/aws"
+import (
+	awsv2 "github.com/aws/aws-sdk-go-v2/aws"

-	input := &sts.AssumeRoleWithSAMLInput{
-		RoleArn:       aws.String(role.RoleARN),
-		PrincipalArn:  aws.String(role.PrincipalARN),
-		SAMLAssertion: aws.String(samlAssertion),
-		DurationSeconds: aws.Int32(func() int32 {
+	input := &sts.AssumeRoleWithSAMLInput{
+		RoleArn:       awsv2.String(role.RoleARN),
+		PrincipalArn:  awsv2.String(role.PrincipalARN),
+		SAMLAssertion: awsv2.String(samlAssertion),
+		DurationSeconds: awsv2.Int32(func() int32 {
 			// Respect requested duration within STS/account limits.
 			if p.config != nil && p.config.Session != nil && p.config.Session.Duration != "" {
 				if duration, err := time.ParseDuration(p.config.Session.Duration); err == nil {
 					return int32(duration.Seconds())
 				}
 			}
 			return 3600
 		}()),
 	}

-	AccessKeyID:     aws.ToString(result.Credentials.AccessKeyId),
-	SecretAccessKey: aws.ToString(result.Credentials.SecretAccessKey),
-	SessionToken:    aws.ToString(result.Credentials.SessionToken),
+	AccessKeyID:     awsv2.ToString(result.Credentials.AccessKeyId),
+	SecretAccessKey: awsv2.ToString(result.Credentials.SecretAccessKey),
+	SessionToken:    awsv2.ToString(result.Credentials.SessionToken),

Also applies to: 227-239, 248-253

---

151-167: Abstract external clients behind interfaces for testability.

Constructing saml2aws and STS clients inline makes unit testing harder. Consider injecting factories/interfaces.

Happy to sketch minimal interfaces and a provider-level struct field to accept test doubles.

Also applies to: 215-224

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

tests/fixtures/scenarios/auth/atmos.yaml (1)

65-71: Schema path must exist and auth schemas must be defined.

The manifest path may be missing, and auth.providers/identities need JSON Schema coverage. This was flagged before; verify and add schemas or update the path.

#!/bin/bash
# Verify schemas and manifest presence
set -e
rg -nP '"auth"\s*:' pkg/datafetcher/schema || true
rg -nP '"providers"|\"identities\"' pkg/datafetcher/schema || true
fd -a 'schemas/atmos/atmos-manifest/1.0/atmos-manifest.json' || true

internal/auth/docs/ARCHITECTURE.md (1)

26-29: Fix incorrect AWS file-management paths in Package Structure.

These entries still reference environment/aws_files.go, but the implementation lives under cloud/aws (e.g., cloud/aws/files.go or cloud/aws/aws_files.go). Update the tree to match the codebase.

Run to confirm the actual paths:

#!/bin/bash
fd -a 'internal/auth/(environment/aws_files.go|cloud/aws/*files.go|cloud/aws/*provider.go)'

Apply:

-├── environment/             # Environment management
-│   ├── aws_files.go         # AWS credentials/config file management
-│   └── merger.go            # Environment variable merging
+├── cloud/                    # Cloud provider implementations
+│   └── aws/
+│       ├── files.go          # AWS credentials/config file management
+│       └── provider.go       # AWS cloud-provider adapter

internal/auth/docs/USER_GUIDE.md (1)

74-75: Annotate GitHub OIDC status.

Per implementation and factory registration, mark as Stable.

-- **GitHub OIDC**: `github/oidc`
+- **GitHub OIDC**: `github/oidc` — Status: Stable

internal/auth/validation/validator.go (5)

34-41: Compute provider/identity pointer maps once; avoid repeated conversions.

Minor perf/readability improvement; reuse converted maps.

-	// Validate providers
-	//nolint:gocritic // rangeValCopy: map stores structs; address of map element can't be taken. Passing copy to factory is intended.
-	for name, provider := range config.Providers {
+	// Precompute pointer maps for reuse.
+	providersPtr := convertProviders(config.Providers)
+	identitiesPtr := convertIdentities(config.Identities)
+
+	// Validate providers.
+	//nolint:gocritic // rangeValCopy: map stores structs; address of map element can't be taken.
+	for name, provider := range config.Providers {
 		if err := v.ValidateProvider(name, &provider); err != nil {
 			return fmt.Errorf("%w: provider %q validation failed: %v", errUtils.ErrInvalidAuthConfig, name, err)
 		}
 	}
 
-	// Validate identities
-
-	for name, identity := range config.Identities {
-		if err := v.ValidateIdentity(name, &identity, convertProviders(config.Providers)); err != nil {
+	// Validate identities.
+	for name, identity := range config.Identities {
+		if err := v.ValidateIdentity(name, &identity, providersPtr); err != nil {
 			return fmt.Errorf("%w: identity %q validation failed: %v", errUtils.ErrInvalidAuthConfig, name, err)
 		}
 	}
 
-	// Validate chains
-	if err := v.ValidateChains(convertIdentities(config.Identities), convertProviders(config.Providers)); err != nil {
+	// Validate chains.
+	if err := v.ValidateChains(identitiesPtr, providersPtr); err != nil {
 		return fmt.Errorf("%w: identity chain validation failed: %v", errUtils.ErrInvalidAuthConfig, err)
 	}

Also applies to: 44-53

---

109-116: Require via for non-user identities and enforce exactly one of provider/identity.

Prevents misconfigured chains and runtime failures.

-	// Validate via configuration - AWS User identities don't require via provider
-	if identity.Kind != "aws/user" && identity.Via != nil {
-		if identity.Via.Provider != "" {
-			if _, exists := providers[identity.Via.Provider]; !exists {
-				return fmt.Errorf("%w: referenced provider %q does not exist", errUtils.ErrInvalidAuthConfig, identity.Via.Provider)
-			}
-		}
-	}
+	// Validate via configuration — required for all non-user identities.
+	if identity.Kind != "aws/user" {
+		if identity.Via == nil {
+			return fmt.Errorf("%w: identity %q requires a via configuration", errUtils.ErrInvalidIdentityConfig, name)
+		}
+		// Exactly one of provider or identity must be set.
+		if (identity.Via.Provider == "" && identity.Via.Identity == "") || (identity.Via.Provider != "" && identity.Via.Identity != "") {
+			return fmt.Errorf("%w: identity %q must specify either via.provider or via.identity", errUtils.ErrInvalidIdentityConfig, name)
+		}
+		if identity.Via.Provider != "" {
+			if _, exists := providers[identity.Via.Provider]; !exists {
+				return fmt.Errorf("%w: referenced provider %q does not exist", errUtils.ErrInvalidIdentityConfig, identity.Via.Provider)
+			}
+		}
+	}

---

137-143: Validate referenced identities exist during graph build.

Reject dangling references early with actionable error.

 		if identity.Via != nil {
 			if identity.Via.Identity != "" {
+				if _, ok := identities[identity.Via.Identity]; !ok {
+					return fmt.Errorf("%w: referenced identity %q does not exist", errUtils.ErrInvalidAuthConfig, identity.Via.Identity)
+				}
 				graph[name] = append(graph[name], identity.Via.Identity)
 			}
 		}

---

160-170: Use provider-scoped sentinel and tighten URL validation.

Return ErrInvalidProviderConfig in provider validators; ensure SAML URL has scheme+host.

 func (v *validator) validateSSOProvider(provider *schema.Provider) error {
 	if provider.StartURL == "" {
-		return fmt.Errorf("%w: start_url is required for AWS SSO provider", errUtils.ErrInvalidAuthConfig)
+		return fmt.Errorf("%w: start_url is required for AWS SSO provider", errUtils.ErrInvalidProviderConfig)
 	}
 	if provider.Region == "" {
-		return fmt.Errorf("%w: region is required for AWS SSO provider", errUtils.ErrInvalidAuthConfig)
+		return fmt.Errorf("%w: region is required for AWS SSO provider", errUtils.ErrInvalidProviderConfig)
 	}
 	return nil
 }
@@
 func (v *validator) validateSAMLProvider(provider *schema.Provider) error {
 	if provider.URL == "" {
-		return fmt.Errorf("%w: url is required for AWS SAML provider", errUtils.ErrInvalidAuthConfig)
+		return fmt.Errorf("%w: url is required for AWS SAML provider", errUtils.ErrInvalidProviderConfig)
 	}
 	if provider.Region == "" {
-		return fmt.Errorf("%w: region is required for AWS SAML provider", errUtils.ErrInvalidAuthConfig)
+		return fmt.Errorf("%w: region is required for AWS SAML provider", errUtils.ErrInvalidProviderConfig)
 	}
-	// Validate URL format
-	if _, err := url.Parse(provider.URL); err != nil {
-		return fmt.Errorf("%w: invalid URL format: %v", errUtils.ErrInvalidAuthConfig, err)
+	// Validate URL format strictly (scheme + host required).
+	u, err := url.ParseRequestURI(provider.URL)
+	if err != nil || u.Scheme == "" || u.Host == "" {
+		return fmt.Errorf("%w: invalid URL format %q", errUtils.ErrInvalidProviderConfig, provider.URL)
 	}
 	return nil
 }
@@
 func (v *validator) validateGitHubOIDCProvider(provider *schema.Provider) error {
 	if provider.Region == "" {
-		return fmt.Errorf("%w: region is required for GitHub OIDC provider", errUtils.ErrInvalidAuthConfig)
+		return fmt.Errorf("%w: region is required for GitHub OIDC provider", errUtils.ErrInvalidProviderConfig)
 	}
 	return nil
 }

Also applies to: 176-191, 194-199

---

237-239: Accept all AWS partitions for role ARN validation.

Don’t hardcode arn:aws; permit aws-cn, aws-us-gov, etc.

-	if !strings.HasPrefix(roleArn, "arn:aws:iam::") {
-		return fmt.Errorf("%w: invalid role ARN format", errUtils.ErrInvalidAuthConfig)
-	}
+	if !strings.HasPrefix(roleArn, "arn:aws") || !strings.Contains(roleArn, ":iam::") || !strings.Contains(roleArn, ":role/") {
+		return fmt.Errorf("%w: invalid role ARN format %q", errUtils.ErrInvalidIdentityConfig, roleArn)
+	}

internal/auth/hooks.go (7)

25-31: Guard against nil inputs instead of silently skipping.

Returning nil when stackInfo/atmosConfig are nil can mask misconfigurations. Fail fast with a wrapped sentinel.

 func TerraformPreHook(atmosConfig *schema.AtmosConfiguration, stackInfo *schema.ConfigAndStacksInfo) error {
-	if !validateAuthConfig(atmosConfig, stackInfo) {
+	if stackInfo == nil {
+		return fmt.Errorf("%w: stackInfo is required", errUtils.ErrInvalidAuthConfig)
+	}
+	if atmosConfig == nil {
+		return fmt.Errorf("%w: atmosConfig is required", errUtils.ErrInvalidAuthConfig)
+	}
+	if !validateAuthConfig(atmosConfig, stackInfo) {
 		log.Debug("No auth configuration found, skipping authentication")
 		return nil
 	}

---

54-58: Validate the auth manager before use.

Catch misconfigurations early.

 	authManager, err := newAuthManager(&authConfig, stackInfo)
 	if err != nil {
 		return err
 	}
+
+	// Validate merged configuration before authenticating.
+	if err := authManager.Validate(); err != nil {
+		return fmt.Errorf("%w: invalid auth configuration: %w", errUtils.ErrInvalidAuthConfig, err)
+	}

---

67-71: Wrap default-identity lookup error with %w.

Preserve the cause per Go idioms and your guidelines.

-		if err != nil {
-			return fmt.Errorf("%w: failed to get default identity: %v", errUtils.ErrDefaultIdentity, err)
-		}
+		if err != nil {
+			return fmt.Errorf("%w: failed to get default identity: %w", errUtils.ErrDefaultIdentity, err)
+		}

---

79-82: Preserve authentication failure cause with %w.

Keeps the error chain intact for troubleshooting.

-	whoami, err := authManager.Authenticate(ctx, targetIdentityName)
-	if err != nil {
-		return fmt.Errorf("%w: failed to authenticate with identity %q: %v", errUtils.ErrAuthenticationFailed, targetIdentityName, err)
-	}
+	whoami, err := authManager.Authenticate(ctx, targetIdentityName)
+	if err != nil {
+		return fmt.Errorf("%w: failed to authenticate with identity %q: %w", errUtils.ErrAuthenticationFailed, targetIdentityName, err)
+	}

---

86-87: Propagate output errors and ensure YAML is written to stdout.

Don’t drop the error; and the util currently only logs. Update both call-site and util.

-	_ = utils.PrintAsYAMLToFileDescriptor(atmosConfig, stackInfo.ComponentEnvSection)
-	return nil
+	if err := utils.PrintAsYAMLToFileDescriptor(atmosConfig, stackInfo.ComponentEnvSection); err != nil {
+		return fmt.Errorf("%w: failed to emit environment configuration", err)
+	}
+	return nil

Also update pkg/utils/yaml_utils.go:

--- a/pkg/utils/yaml_utils.go
+++ b/pkg/utils/yaml_utils.go
@@
+import (
+  "fmt"
+  "os"
+)
@@
-  log.Debug("PrintAsYAMLToFileDescriptor", "data", y)
-  return nil
+  // Data/results must go to stdout for piping.
+  if _, err := fmt.Fprint(os.Stdout, y); err != nil {
+    return fmt.Errorf("write to stdout failed: %w", err)
+  }
+  return nil

---

121-139: Nil-safe log level parsing with sane defaults.

Avoid NPEs when Logs/Auth.Logs are nil; default to current/info; only set parsed levels when valid.

-func getConfigLogLevels(atmosConfig *schema.AtmosConfiguration) (log.Level, log.Level) {
-	if atmosConfig == nil {
-		return log.InfoLevel, log.InfoLevel
-	}
-	atmosLevel := log.GetLevel()
-	if atmosConfig.Logs.Level != "" {
-		if l, err := log.ParseLevel(atmosConfig.Logs.Level); err == nil {
-			atmosLevel = l
-		}
-	}
-	// Determine auth log level (fallback to atmos level).
-	authLevel := atmosLevel
-	if atmosConfig.Auth.Logs.Level != "" {
-		if l, err := log.ParseLevel(atmosConfig.Auth.Logs.Level); err == nil {
-			authLevel = l
-		}
-	}
-	return atmosLevel, authLevel
-}
+func getConfigLogLevels(atmosConfig *schema.AtmosConfiguration) (log.Level, log.Level) {
+	// Base on current level; fall back to info when config is nil.
+	current := log.GetLevel()
+	if atmosConfig == nil {
+		return current, current
+	}
+	atmosLevel := current
+	if atmosConfig.Logs != nil && atmosConfig.Logs.Level != "" {
+		if l, err := log.ParseLevel(atmosConfig.Logs.Level); err == nil {
+			atmosLevel = l
+		}
+	}
+	authLevel := atmosLevel
+	if atmosConfig.Auth.Logs != nil && atmosConfig.Auth.Logs.Level != "" {
+		if l, err := log.ParseLevel(atmosConfig.Auth.Logs.Level); err == nil {
+			authLevel = l
+		}
+	}
+	return atmosLevel, authLevel
+}

---

43-46: Preserve the original decode error with %w.

Use %w for the cause to keep the error chain.

-	err := mapstructure.Decode(stackInfo.ComponentAuthSection, &authConfig)
-	if err != nil {
-		return fmt.Errorf("%w: failed to decode component auth config: %v", errUtils.ErrInvalidAuthConfig, err)
-	}
+	err := mapstructure.Decode(stackInfo.ComponentAuthSection, &authConfig)
+	if err != nil {
+		return fmt.Errorf("%w: failed to decode component auth config: %w", errUtils.ErrInvalidAuthConfig, err)
+	}

tests/fixtures/scenarios/auth/atmos.yaml (2)

14-14: Strip trailing whitespace (yamllint errors).

-      
+
-        
+
-        
+
-        
+

Also applies to: 25-25, 34-34, 41-41

---

11-13: Clarify region precedence; avoid duplicate sources of truth.

If provider sets region, omit it from user.credentials unless schema mandates it.

-        region: us-east-1
+        # region inherited from provider unless schema requires override
+        # region: us-east-1

Also applies to: 36-41

internal/auth/docs/ARCHITECTURE.md (1)

9-9: Add fenced code languages to satisfy markdownlint (MD040).

Declare languages for the directory tree and the flow diagram.

-```
+```text
@@
-```
+```text
 Component Request → Identity → Provider Chain → Root Provider

Also applies to: 81-84

</blockquote></details>
<details>
<summary>internal/auth/docs/USER_GUIDE.md (2)</summary><blockquote>

`33-36`: **Clarify provider region requirements.**

Region is also required for AWS SSO and AWS SAML per validator.


```diff
 Notes:
 
-- Region is required for the GitHub OIDC provider and is validated at construction time.
+- Region is required for GitHub OIDC, AWS SSO, and AWS SAML providers and is validated during construction.

---

396-401: Use proper headings instead of bold labels (MD036).

Convert emphasized lines to headings.

-**Configuration Validation Errors**
+#### Configuration Validation Errors
@@
-**Authentication Failures**
+#### Authentication Failures
@@
-**Permission Errors**
+#### Permission Errors
@@
-**Environment Variable Issues**
+#### Environment Variable Issues

Also applies to: 402-414, 415-424, 425-434

internal/auth/hooks.go (2)

38-42: Fix godot lints: end comments with periods.

golangci-lint (godot) will fail on these.

-// TODO: verify if we need to use Decode, or if we can use the merged auth config directly
-// Use the merged auth configuration from stackInfo
-// ComponentAuthSection already contains the deep-merged auth config from component + inherits + atmos.yaml
-// Converted to typed struct when needed
+// TODO: verify if we need to use Decode, or if we can use the merged auth config directly.
+// Use the merged auth configuration from stackInfo.
+// ComponentAuthSection already contains the deep-merged auth config from component + inherits + atmos.yaml.
+// Converted to typed struct when needed.
@@
-// Skip if no auth config (check the merged config, not the original)
+// Skip if no auth config (check the merged config, not the original).
@@
-// Determine target identity: stack info identity (CLI flag) or default identity
+// Determine target identity: stack info identity (CLI flag) or default identity.
@@
-// This is set by the CLI Flag
+// This is set by the CLI flag.
@@
-// Create auth manager components
+// Create auth manager components.
@@
-// Create auth manager with merged configuration and stack info (so identities can mutate it)
+// Create auth manager with merged configuration and stack info (so identities can mutate it).

Also applies to: 48-48, 59-59, 63-63, 104-104, 108-108

---

78-80: Optional: Add a timeout to authentication.

Prevents hanging in CI/automation.

-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Minute)
+	defer cancel()

Add:

import "time"

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

tests/fixtures/scenarios/auth/atmos.yaml (2)

65-71: Verify schemas exist and model new auth fields.

The manifest path and schema defs for auth.providers/identities must exist and align with this fixture. Please verify and add if missing.

#!/bin/bash
set -euo pipefail
echo "== Check atmos manifest path =="
fd -a 'schemas/atmos/atmos-manifest/1.0/atmos-manifest.json' || true

echo "== Check schema contains auth.providers & auth.identities =="
rg -nP '"auth"\s*:' pkg/datafetcher/schema || true
rg -nP '"providers"|\"identities\"' pkg/datafetcher/schema || true

---

35-41: Good: test credentials are no longer AWS-shaped.

This avoids tripping secret scanners.

internal/auth/manager.go (6)

395-401: Don’t ignore provider.PreAuthenticate errors; log and continue.

Capture the error to aid troubleshooting without failing the flow.

-    if provider, exists := m.providers[m.chain[0]]; exists {
-      _ = provider.PreAuthenticate(m)
-    }
+    if provider, exists := m.providers[m.chain[0]]; exists {
+      if err := provider.PreAuthenticate(m); err != nil {
+        log.Warn("PreAuthenticate failed", "provider", m.chain[0], "error", err)
+      }
+    }

---

606-613: Provider resolution for Whoami: ensure root provider even when chain not set.

Fallback to chain[0] or rebuild the chain to avoid mislabeling for nested identities.

-func (m *manager) buildWhoamiInfo(identityName string, creds types.ICredentials) *types.WhoamiInfo {
-  providerName := m.getProviderForIdentity(identityName)
+func (m *manager) buildWhoamiInfo(identityName string, creds types.ICredentials) *types.WhoamiInfo {
+  providerName := m.getProviderForIdentity(identityName)
+  if len(m.chain) > 0 && m.chain[0] != "" {
+    providerName = m.chain[0]
+  } else if chain, err := m.buildAuthenticationChain(identityName); err == nil && len(chain) > 0 {
+    providerName = chain[0]
+  }

---

620-625: Merge provider env vars into WhoamiInfo.

Currently only identity env is included; add provider env for completeness.

-  // Get environment variables
+  // Get environment variables (provider + identity).
+  if providerName != "" {
+    if p, ok := m.providers[providerName]; ok {
+      if penv, err := p.Environment(); err == nil && len(penv) > 0 {
+        if info.Environment == nil {
+          info.Environment = map[string]string{}
+        }
+        for k, v := range penv {
+          info.Environment[k] = v
+        }
+      }
+    }
+  }
   if identity, exists := m.identities[identityName]; exists {
     if env, err := identity.Environment(); err == nil {
       info.Environment = env
     }
   }

---

442-451: Guard against out-of-range startIndex in retrieveCachedCredentials.

Defensive check prevents potential panic on future refactors.

 func (m *manager) retrieveCachedCredentials(chain []string, startIndex int) (types.ICredentials, error) {
+  if startIndex < 0 || startIndex >= len(chain) {
+    return nil, fmt.Errorf("%w: invalid startIndex %d", errUtils.ErrInvalidAuthConfig, startIndex)
+  }
   identityName := chain[startIndex]
   currentCreds, err := m.credentialStore.Retrieve(identityName)
   if err != nil {
     return nil, err
   }

---

22-23: Doc comment polish (typo + periods for godot).

Fix casing and end sentences with periods.

-// manager implements the Authmanager interface.
+// manager implements the AuthManager interface.
@@
-// NewAuthManager creates a new Authmanager instance.
+// NewAuthManager creates a new AuthManager instance.
@@
-// getProviderForIdentity returns the provider name for the given identity
+// getProviderForIdentity returns the provider name for the given identity.
@@
-// GetProviderForIdentity returns the provider name for the given identity
+// GetProviderForIdentity returns the provider name for the given identity.
@@
-// GetProviderKindForIdentity returns the provider kind for the given identity
+// GetProviderKindForIdentity returns the provider kind for the given identity.
@@
-// buildAuthenticationChain builds the authentication chain from target identity to source provider
-// Returns a slice where [0] is the provider name, [1..n] are identity names in authentication order
+// buildAuthenticationChain builds the authentication chain from target identity to source provider.
+// Returns a slice where [0] is the provider name, [1..n] are identity names in authentication order.
@@
-// GetConfig returns the config.
+// GetConfig returns the current stack info pointer.

Also applies to: 35-36, 238-240, 266-268, 272-274, 531-533, 429-433

---

488-491: Optional: standardize log key names (snake_case).

E.g., chain_length/start_index/provider to match docs.

Also applies to: 89-90, 450-472

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: Benbentwo
PR: cloudposse/atmos#1452
File: internal/auth/manager.go:0-0
Timestamp: 2025-09-08T16:26:41.558Z
Learning: charmbracelet/log library supports printf-style logging methods including Debugf, Infof, Warnf, Errorf, and Fatalf in addition to structured logging methods.

tests/test-cases/core.yaml (1)

81-81: Tighten the regex to be precise.

The dot is a regex wildcard; anchor and escape to avoid accidental matches.

-        - "Opening default browser to 'https://atmos.tools'."
+        - "^Opening default browser to 'https://atmos\\.tools'\\.$"

tests/snapshots/TestCLICommands_atmos_auth_exec_--help.stderr.golden (2)

4-4: Fix glob text: '*.yaml' should be '*.yaml'

The first path shows '//*.yaml' while the parenthetical shows '/*.yaml'. Make them consistent to avoid confusion and false negatives when matching snapshots.

-failed to load atmos config: failed to find a match for the import '/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks/**/*****.yaml' ('/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks' + '**/*.yaml')
+failed to load atmos config: failed to find a match for the import '/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks/**/*.yaml' ('/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks' + '**/*.yaml')

---

2-6: Trim whitespace and standardize error prefix

There are trailing spaces and extra blank lines. Prefer a compact, deterministic message and use “Error:” (no Markdown header) for stderr.

-
-# Error                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                 
-                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        
-failed to load atmos config: failed to find a match for the import '/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks/**/*****.yaml' ('/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks' + '**/*.yaml')                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  
-
-
+Error:
+failed to load atmos config: failed to find a match for the import '/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks/**/*.yaml' ('/absolute/path/to/repo/tests/fixtures/scenarios/auth/stacks' + '**/*.yaml')

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

internal/auth/validation/validator.go (5)

31-31: Fix double %w (go vet).

Each fmt.Errorf must contain at most one %w; see earlier diff for replacements to %v.

Also applies to: 39-39, 45-45, 182-182

---

127-138: Validate referenced identities exist when building the graph.

Rejects dangling edges early.

 for name, identity := range identities {
   if identity.Via != nil {
     if identity.Via.Identity != "" {
+      if _, ok := identities[identity.Via.Identity]; !ok {
+        return fmt.Errorf("%w: referenced identity %q does not exist", errUtils.ErrInvalidAuthConfig, identity.Via.Identity)
+      }
       graph[name] = append(graph[name], identity.Via.Identity)
     }
   }
 }

---

189-195: Use provider-scoped sentinel for GitHub OIDC errors.

 if provider.Region == "" {
-  return fmt.Errorf("%w: region is required for GitHub OIDC provider", errUtils.ErrInvalidAuthConfig)
+  return fmt.Errorf("%w: region is required for GitHub OIDC provider", errUtils.ErrInvalidProviderConfig)
 }

---

155-166: Use provider-scoped sentinel for SSO provider errors.

Aligns with error taxonomy.

 if provider.StartURL == "" {
-  return fmt.Errorf("%w: start_url is required for AWS SSO provider", errUtils.ErrInvalidAuthConfig)
+  return fmt.Errorf("%w: start_url is required for AWS SSO provider", errUtils.ErrInvalidProviderConfig)
 }
 
 if provider.Region == "" {
-  return fmt.Errorf("%w: region is required for AWS SSO provider", errUtils.ErrInvalidAuthConfig)
+  return fmt.Errorf("%w: region is required for AWS SSO provider", errUtils.ErrInvalidProviderConfig)
 }

---

102-110: Require via for non-user identities and enforce exactly one of provider/identity.

Prevents misconfiguration and later runtime errors.

-// Validate via configuration - AWS User identities don't require via provider
-if identity.Kind != "aws/user" && identity.Via != nil {
-  if identity.Via.Provider != "" {
-    if _, exists := providers[identity.Via.Provider]; !exists {
-      return fmt.Errorf("%w: referenced provider %q does not exist", errUtils.ErrInvalidAuthConfig, identity.Via.Provider)
-    }
-  }
-}
+// Non-user identities must specify via, with exactly one of provider or identity.
+if identity.Kind != "aws/user" {
+  if identity.Via == nil {
+    return fmt.Errorf("%w: identity %q requires a via configuration", errUtils.ErrInvalidIdentityConfig, name)
+  }
+  hasProv := identity.Via.Provider != ""
+  hasIdent := identity.Via.Identity != ""
+  if hasProv == hasIdent {
+    return fmt.Errorf("%w: identity %q must specify exactly one of via.provider or via.identity", errUtils.ErrInvalidIdentityConfig, name)
+  }
+  if hasProv {
+    if _, exists := providers[identity.Via.Provider]; !exists {
+      return fmt.Errorf("%w: referenced provider %q does not exist", errUtils.ErrInvalidAuthConfig, identity.Via.Provider)
+    }
+  }
+}

internal/auth/identities/aws/permission_set.go (1)

3-16: Use SSO access token correctly and discover the actual role name (with pagination).

• Don’t overload AWSCredentials.AccessKeyID with the SSO token; pass the token via AccessToken params and drop the fake credentials provider.
• Permission set name ≠ role name; use ListAccountRoles and paginate accounts/roles.

Apply:

@@
 import (
 	"context"
 	"fmt"
 	"time"
+	"strings"
 
 	awssdk "github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/config"
 	"github.com/aws/aws-sdk-go-v2/service/sso"
@@
-	// Create AWS config using the base credentials (SSO access token)
-	cfg, err := config.LoadDefaultConfig(ctx,
-		config.WithRegion(awsBase.Region),
-		config.WithCredentialsProvider(awssdk.CredentialsProviderFunc(func(ctx context.Context) (awssdk.Credentials, error) {
-			return awssdk.Credentials{
-				AccessKeyID: awsBase.AccessKeyID, // This is actually the SSO access token
-			}, nil
-		})),
-	)
+	// Create AWS config using the base region.
+	cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(awsBase.Region))
 	if err != nil {
 		return nil, fmt.Errorf("%w: failed to load AWS config: %v", errUtils.ErrInvalidIdentityConfig, err)
 	}
@@
-	// List accounts to find the target account ID
-	accountsResp, err := ssoClient.ListAccounts(ctx, &sso.ListAccountsInput{
-		AccessToken: awssdk.String(awsBase.AccessKeyID), // SSO access token
-	})
-	if err != nil {
-		return nil, fmt.Errorf("%w: failed to list accounts: %v", errUtils.ErrAwsAuth, err)
-	}
-
-	if okAccountName {
-		for _, account := range accountsResp.AccountList {
-			if awssdk.ToString(account.AccountName) == accountName {
-				accountID = awssdk.ToString(account.AccountId)
-				break
-			}
-		}
-		if accountID == "" {
-			return nil, fmt.Errorf("%w: account %q not found", errUtils.ErrAwsAuth, accountName)
-		}
-	}
+	// Resolve account ID by name if needed (paginated).
+	if accountID == "" && accountName != "" {
+		var nextToken *string
+		for {
+			accountsResp, err := ssoClient.ListAccounts(ctx, &sso.ListAccountsInput{
+				AccessToken: awssdk.String(awsBase.SessionToken),
+				NextToken:   nextToken,
+				MaxResults:  awssdk.Int32(1000),
+			})
+			if err != nil {
+				return nil, fmt.Errorf("%w: failed to list accounts: %v", errUtils.ErrAwsAuth, err)
+			}
+			for _, account := range accountsResp.AccountList {
+				if awssdk.ToString(account.AccountName) == accountName {
+					accountID = awssdk.ToString(account.AccountId)
+					break
+				}
+			}
+			if accountID != "" || accountsResp.NextToken == nil {
+				break
+			}
+			nextToken = accountsResp.NextToken
+		}
+		if accountID == "" {
+			return nil, fmt.Errorf("%w: account %q not found", errUtils.ErrAwsAuth, accountName)
+		}
+	}
 
-	// Get role credentials for the permission set
-	roleCredsResp, err := ssoClient.GetRoleCredentials(ctx, &sso.GetRoleCredentialsInput{
-		AccountId:   awssdk.String(accountID),
-		RoleName:    awssdk.String(permissionSetName),
-		AccessToken: awssdk.String(awsBase.AccessKeyID),
-	})
+	// Discover the actual role name for the permission set (paginated).
+	var roleName string
+	var nextToken *string
+	for {
+		rolesResp, err := ssoClient.ListAccountRoles(ctx, &sso.ListAccountRolesInput{
+			AccessToken: awssdk.String(awsBase.SessionToken),
+			AccountId:   awssdk.String(accountID),
+			NextToken:   nextToken,
+			MaxResults:  awssdk.Int32(1000),
+		})
+		if err != nil {
+			return nil, fmt.Errorf("%w: failed to list account roles: %v", errUtils.ErrAwsAuth, err)
+		}
+		for _, r := range rolesResp.RoleList {
+			rn := awssdk.ToString(r.RoleName)
+			if rn == permissionSetName || strings.HasPrefix(rn, "AWSReservedSSO_"+permissionSetName+"_") {
+				roleName = rn
+				break
+			}
+		}
+		if roleName != "" || rolesResp.NextToken == nil {
+			break
+		}
+		nextToken = rolesResp.NextToken
+	}
+	if roleName == "" {
+		return nil, fmt.Errorf("%w: no role found for permission set %q in account %q", errUtils.ErrAwsAuth, permissionSetName, accountName)
+	}
+
+	// Get role credentials using the discovered role name.
+	roleCredsResp, err := ssoClient.GetRoleCredentials(ctx, &sso.GetRoleCredentialsInput{
+		AccountId:   awssdk.String(accountID),
+		RoleName:    awssdk.String(roleName),
+		AccessToken: awssdk.String(awsBase.SessionToken),
+	})
 	if err != nil {
 		return nil, fmt.Errorf("%w: failed to get role credentials: %v", errUtils.ErrAuthenticationFailed, err)
 	}

Note: Ensure the SSO provider populates AWSCredentials.SessionToken with the SSO access token; otherwise introduce a dedicated SSOCredentials/OIDCCredentials type.

Also applies to: 81-90, 94-125

pkg/schema/schema.go (2)

457-457: Wire the identity flag/env end-to-end (verify).

You added Identity to ArgsAndFlagsInfo and ConfigAndStacksInfo; ensure a Cobra flag exists and viper.BindEnv("identity","ATMOS_IDENTITY") is set so precedence is flags > env > config.

Run:

#!/bin/bash
rg -nP --type=go -C2 '\.(Persistent)?Flags\(\)\.(String|StringVar|StringP|StringVarP)\([^"]*"identity"' 
rg -nP --type=go 'viper\.BindEnv\(\s*"identity"\s*,\s*"ATMOS_IDENTITY"\s*\)'
rg -nP --type=go '\bATMOS_IDENTITY\b'

Also applies to: 538-538

---

974-979: Fix doc comments to pass golangci-lint (godot).

Add trailing periods to exported-type comments.

Apply:

-// AuthConfig defines the authentication configuration structure
+// AuthConfig defines the authentication configuration structure.
@@
-// Provider defines an authentication provider configuration
+// Provider defines an authentication provider configuration.
@@
-// SessionConfig defines session configuration for providers
+// SessionConfig defines session configuration for providers.
@@
-// IdentityVia defines how an identity connects to a provider or other identity
+// IdentityVia defines how an identity connects to a provider or other identity.
@@
-// EnvironmentVariable defines an environment variable with preserved case sensitivity
+// EnvironmentVariable defines an environment variable with preserved case sensitivity.
@@
-// ComponentAuthConfig defines auth configuration at the component level
+// ComponentAuthConfig defines auth configuration at the component level.

Also applies to: 981-995, 996-1000, 1012-1017, 1018-1023, 1024-1029

tests/fixtures/scenarios/atmos-auth/stacks/deploy/component-auth-configured.yaml (2)

15-18: Identity is missing kind; likely to fail schema/validation.

Per schema.Identity, kind is expected. If omission is intentional for a merge-only fixture, disregard; otherwise add the concrete identity kind.

         identities:
           identity-oidc:
+            kind: aws/permission_set  # or aws/assume_role, as appropriate
             via:
               provider: provider-oidc

---

19-19: Fix trailing spaces and ensure newline at EOF.

YAMLlint flags trailing spaces and missing final newline.

-          
+

tests/fixtures/scenarios/atmos-auth-invalid/atmos.yaml (1)

13-13: Remove trailing whitespace.

Keeps fixtures clean and avoids lint noise.

-      
+

tests/fixtures/scenarios/atmos-auth/stacks/deploy/nonprod.yaml (1)

9-9: Add newline at EOF.

Silences YAMLlint new-line-at-end-of-file.

-        component: mock_caller_identity
+        component: mock_caller_identity
+

tests/test-cases/auth-merge.yaml (1)

21-26: Make assertion less brittle by matching structured output.

Instead of matching a YAML snippet, consider asserting JSON keys/values (if available via -o json) to reduce whitespace/indent sensitivity.

tests/test-cases/auth-default-identity.yaml (2)

115-116: Align test name with args (admin-1).

Prevents confusion when scanning CI logs.

-  - name: atmos auth whoami --identity admin (multiple defaults)
+  - name: atmos auth whoami --identity admin-1 (multiple defaults)

---

3-148: Deduplicate common test fields with YAML anchors.

DRY the repeated workdir/command/env to make edits cheaper.

Example:

 tests:
+  - &defaults
+    workdir: "fixtures/scenarios/atmos-auth/"
+    command: "atmos"
   - name: atmos auth validate
-    workdir: "fixtures/scenarios/atmos-auth/"
-    command: "atmos"
+    <<: *defaults
     args:
       - "auth"
       - "validate"

tests/snapshots/TestCLICommands_atmos_auth_env_--format_export_--identity_test-user.stderr.golden (1)

4-4: Snapshot includes absolute, OS-specific paths; consider normalizing.

Normalize or redact repo paths in the produced error (or in test harness) to avoid cross-platform flakiness.

tests/test-cases/auth-cli.yaml (1)

3-337: Reduce duplication with YAML anchors for workdir/command and common env.

Keeps the 18-case suite maintainable.

 tests:
+  - &defaults
+    workdir: "fixtures/scenarios/atmos-auth/"
+    command: "atmos"
   - name: atmos auth validate
-    workdir: "fixtures/scenarios/atmos-auth/"
-    command: "atmos"
+    <<: *defaults
     args:
       - "auth"
       - "validate"

internal/auth/types/whoami.go (1)

13-13: Consider masking or omitting sensitive env values in Whoami output.

Environment can include AWS_SESSION_TOKEN, etc. Either exclude by default or mask values at the CLI layer.

tests/test-cases/auth-validation.yaml (2)

10-10: Trim trailing spaces to satisfy linters.

Removes YAMLlint errors.

-  
+

Also applies to: 16-16, 21-21, 39-39, 48-48, 55-55, 58-58

---

22-37: Route human-readable output to stderr per guidelines.

“Validating authentication configuration” and the success line should go to stderr; stdout should be data-only. Update the command implementation and adjust test expectations accordingly.

Do you want me to open a follow-up to flip these streams and update snapshots?

internal/auth/types/github_oidc_credentials.go (1)

23-26: End inline comment with a period (godot).

Satisfy lint rule requiring comments to end with periods.

 func (c *OIDCCredentials) BuildWhoamiInfo(info *WhoamiInfo) {
-	// No additional fields to populate for generic OIDC creds
+	// No additional fields to populate for generic OIDC creds.
 }

tests/fixtures/scenarios/atmos-auth/atmos.yaml (1)

10-10: Trim trailing spaces to keep fixtures lint-clean.

-  
+

Also applies to: 16-16, 21-21, 39-39, 48-48, 55-55, 58-58

internal/auth/validation/validator.go (2)

65-66: Message consistency: quote dynamic values and end sentences with periods.

Optional polish; improves UX.

Also applies to: 88-89, 123-124

---

28-28: Nit: comment periods and section headers.

Add trailing periods and unify comment style.

Also applies to: 35-37, 43-45

internal/auth/identities/aws/permission_set.go (1)

61-69: Add trailing periods to comments to satisfy godot.

Several inline comments lack trailing periods (e.g., Lines 61, 68, 81, 94, 117, 127, 143). Please append periods.

Also applies to: 81-82, 94-97, 117-118, 127-128, 141-144

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.