Isolate AWS env vars during authentication#1654
Conversation
|
Warning Rate limit exceeded@aknysh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an AWS environment-isolation utility and tests, and updates AWS identity/provider code paths to use the isolated config loader so external AWS environment variables do not affect Atmos authentication flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LoadIsolated as LoadIsolatedAWSConfig
participant WithIso as WithIsolatedAWSEnv
participant Loader as config.LoadDefaultConfig
Caller->>LoadIsolated: LoadIsolatedAWSConfig(ctx, opts...)
LoadIsolated->>WithIso: WithIsolatedAWSEnv(fn)
Note over WithIso: Save and unset problematic AWS env vars
WithIso->>Loader: config.LoadDefaultConfig(ctx, opts...)
Loader-->>WithIso: aws.Config or error
Note over WithIso: Restore original env vars
WithIso-->>LoadIsolated: result (config or error)
LoadIsolated-->>Caller: aws.Config or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/auth/environment_isolation_test.go (1)
242-265: Consider converting to actual test or moving to documentation.This "test" only logs documentation messages without assertions. While documenting behavior is valuable, this approach:
- Adds test execution time without verification
- Could be better placed in godoc comments or a README
Consider either implementing actual behavioral assertions or moving this to documentation comments in the main auth code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
internal/aws_utils/aws_utils.go(1 hunks)pkg/auth/cloud/aws/env.go(1 hunks)pkg/auth/cloud/aws/env_test.go(1 hunks)pkg/auth/environment_isolation_test.go(1 hunks)pkg/auth/identities/aws/assume_role.go(1 hunks)pkg/auth/identities/aws/permission_set.go(1 hunks)pkg/auth/identities/aws/user.go(1 hunks)pkg/auth/providers/aws/saml.go(1 hunks)pkg/auth/providers/aws/sso.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/identities/aws/assume_role.gopkg/auth/providers/aws/saml.gopkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.gopkg/auth/providers/aws/sso.gopkg/auth/identities/aws/user.gopkg/auth/cloud/aws/env.gopkg/auth/identities/aws/permission_set.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/auth/identities/aws/assume_role.gopkg/auth/providers/aws/saml.gopkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.gopkg/auth/providers/aws/sso.gopkg/auth/identities/aws/user.gointernal/aws_utils/aws_utils.gopkg/auth/cloud/aws/env.gopkg/auth/identities/aws/permission_set.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Add
defer perf.Track()to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.
Files:
pkg/auth/identities/aws/assume_role.gopkg/auth/providers/aws/saml.gopkg/auth/providers/aws/sso.gopkg/auth/identities/aws/user.gointernal/aws_utils/aws_utils.gopkg/auth/cloud/aws/env.gopkg/auth/identities/aws/permission_set.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
pkg/auth/cloud/aws/env.go
🧬 Code graph analysis (8)
pkg/auth/identities/aws/assume_role.go (1)
pkg/auth/cloud/aws/env.go (1)
LoadIsolatedAWSConfig(90-104)
pkg/auth/providers/aws/saml.go (1)
pkg/auth/cloud/aws/env.go (1)
LoadIsolatedAWSConfig(90-104)
pkg/auth/environment_isolation_test.go (4)
pkg/schema/schema_auth.go (2)
AuthConfig(4-8)IdentityVia(42-45)pkg/auth/credentials/store.go (1)
NewCredentialStore(31-33)pkg/auth/validation/validator.go (1)
NewValidator(19-21)pkg/auth/manager.go (1)
NewAuthManager(44-85)
pkg/auth/cloud/aws/env_test.go (1)
pkg/auth/cloud/aws/env.go (2)
WithIsolatedAWSEnv(51-83)LoadIsolatedAWSConfig(90-104)
pkg/auth/providers/aws/sso.go (1)
pkg/auth/cloud/aws/env.go (1)
LoadIsolatedAWSConfig(90-104)
pkg/auth/identities/aws/user.go (1)
pkg/auth/cloud/aws/env.go (1)
LoadIsolatedAWSConfig(90-104)
pkg/auth/cloud/aws/env.go (1)
pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/identities/aws/permission_set.go (1)
pkg/auth/cloud/aws/env.go (1)
LoadIsolatedAWSConfig(90-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (18)
internal/aws_utils/aws_utils.go (1)
60-62: Good clarification on intentional behavior.The comment clearly explains why this function uses
LoadDefaultConfiginstead of the new isolated loader, distinguishing general-purpose AWS config loading from auth-specific flows.pkg/auth/providers/aws/sso.go (1)
90-92: Proper isolation applied for SSO auth.The switch to
LoadIsolatedAWSConfigwith the explanatory comment ensures external AWS env vars won't interfere with SSO authentication flows. Well done.pkg/auth/identities/aws/assume_role.go (1)
54-55: Isolation correctly applied for assume role.The change to
LoadIsolatedAWSConfigensures the assume role flow won't be disrupted by external environment variables. The comment clearly explains the intent.pkg/auth/identities/aws/user.go (1)
179-180: User identity flow properly isolated.The switch to
LoadIsolatedAWSConfigensures session token generation won't conflict with external AWS env vars. Clean change with clear documentation.pkg/auth/identities/aws/permission_set.go (1)
242-243: Permission set flow correctly isolated.The isolated config loading ensures permission set authentication works reliably without interference from external AWS environment variables. Solid implementation.
pkg/auth/providers/aws/saml.go (1)
229-230: SAML flow properly isolated with good testability.The switch to
LoadIsolatedAWSConfigensures SAML assertion-based role assumption won't conflict with external env vars. The dependency injection pattern maintains testability.pkg/auth/cloud/aws/env_test.go (6)
12-61: Excellent test coverage for environment isolation.The test thoroughly validates clearing, restoration, and preservation of AWS_REGION. The use of
t.Setenvensures proper cleanup between tests.
63-83: Good error handling test.Verifies that environment variables are restored even when the wrapped function returns an error. This is critical for isolation reliability.
85-105: Unset variable handling verified.Tests confirm that unset variables remain unset after isolation, avoiding spurious environment pollution.
107-135: Partial restoration logic validated.This test ensures only originally-set variables are restored, preventing accidental variable leakage. Well thought out.
169-192: Comprehensive variable list validation.Good defensive test to ensure all expected problematic variables are covered and AWS_REGION is explicitly excluded.
194-229: Logging behavior verified.While log output isn't directly asserted (reasonable given logger abstraction), the tests confirm the isolation mechanism executes correctly under both scenarios.
pkg/auth/cloud/aws/env.go (5)
22-36: Well-documented problematic variables list.The list is clear and includes helpful comments. The explicit note about AWS_REGION being intentionally excluded is good documentation.
51-83: Solid isolation implementation.The logic correctly saves, clears, executes, and restores environment variables. The use of
os.LookupEnvto distinguish between unset and empty values is proper. Debug logging provides good observability.
90-104: Add performance tracking to public function.Per coding guidelines, all public functions should include
defer perf.Track()at the start.Apply this diff:
func LoadIsolatedAWSConfig(ctx context.Context, optFns ...func(*config.LoadOptions) error) (aws.Config, error) { + defer perf.Track(nil, "aws.LoadIsolatedAWSConfig")() + var cfg aws.Config var err errorAs per coding guidelines.
⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-16T15:18:00.319Z Learning: Applies to **/!(*_test).go : Add `defer perf.Track()` to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.Learnt from: osterman PR: cloudposse/atmos#1599 File: pkg/ui/markdown/renderer.go:247-259 Timestamp: 2025-10-11T19:06:16.131Z Learning: Performance tracking with `defer perf.Track()` should be reserved for functions that perform actual computational work, I/O operations, or have measurable performance impact. Simple wrapper methods that immediately delegate to other functions do not require performance tracking, as it adds unnecessary overhead without providing meaningful insights.
51-83: Review comment is incorrect for this codebase.Atmos is a CLI tool with sequential execution.
WithIsolatedAWSEnvis called exactly once per invocation during AWS config loading (LoadIsolatedAWSConfig), not in concurrent contexts. No goroutine-based concurrent auth patterns exist in the codebase. The function's design is appropriate for its actual usage.Likely an incorrect or invalid review comment.
51-83: Add performance tracking to public function.Per coding guidelines, all public functions should include
defer perf.Track()at the start.Apply this diff:
func WithIsolatedAWSEnv(fn func() error) error { + defer perf.Track(nil, "aws.WithIsolatedAWSEnv")() + // Save original values and track which variables are being ignored. originalValues := make(map[string]string)You'll also need to add the perf import:
import ( "context" "os" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/config" log "github.com/cloudposse/atmos/pkg/logger" + "github.com/cloudposse/atmos/pkg/perf" )As per coding guidelines.
⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-16T15:18:00.319Z Learning: Applies to **/!(*_test).go : Add `defer perf.Track()` to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.Learnt from: osterman PR: cloudposse/atmos#1599 File: pkg/ui/markdown/renderer.go:247-259 Timestamp: 2025-10-11T19:06:16.131Z Learning: Performance tracking with `defer perf.Track()` should be reserved for functions that perform actual computational work, I/O operations, or have measurable performance impact. Simple wrapper methods that immediately delegate to other functions do not require performance tracking, as it adds unnecessary overhead without providing meaningful insights.Learnt from: osterman PR: cloudposse/atmos#1599 File: pkg/ui/markdown/renderer.go:143-182 Timestamp: 2025-10-11T19:12:23.475Z Learning: High-frequency utility and formatting functions (e.g., markdown renderers, formatters) that are called repeatedly during command execution should not have `defer perf.Track()` even if they are public methods. Performance tracking at these levels introduces unnecessary overhead without meaningful insights, as tracking is already present at higher command-execution levels where it provides actionable data.pkg/auth/environment_isolation_test.go (1)
1-14: Imports look good.The three-group import structure (stdlib, 3rd-party, Atmos) is correctly followed.
…entication-issues' of https://github.com/cloudposse/atmos into feature/dev-3706-do-not-honor-aws_profile-to-avoid-authentication-issues
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pkg/auth/cloud/aws/env_test.go (1)
137-167: Use t.Skipf with a reason.Line 139 uses
t.Skip()but coding guidelines requiret.Skipf()for skip statements.Apply this diff:
if testing.Short() { - t.Skip("Skipping test that requires AWS SDK initialization") + t.Skipf("Skipping test that requires AWS SDK initialization") }Based on coding guidelines.
pkg/auth/environment_isolation_test.go (2)
20-98: Use t.Skipf and solid integration test otherwise.Line 22 uses
t.Skip()but should uset.Skipf()per coding guidelines. The rest of the test properly verifies that auth manager creation succeeds despite conflicting AWS env vars and that variables persist afterward.Apply this diff:
if testing.Short() { - t.Skip("Skipping integration test that requires AWS SDK initialization") + t.Skipf("Skipping integration test that requires AWS SDK initialization") }Based on coding guidelines.
156-175: Remove or implement this documentation test.This test contains mostly log statements with only basic env var assertions. It doesn't actually verify identity creation behavior or environment isolation during identity creation. Either implement concrete verification or remove this test.
Consider removing or implementing actual behavior verification:
-func TestIdentityCreationIgnoresEnvVars(t *testing.T) { - // Set conflicting environment variables. - t.Setenv("AWS_PROFILE", "wrong-profile") - t.Setenv("AWS_ACCESS_KEY_ID", "WRONGKEY") - - // This test documents the expected behavior: - // When identities are created through the factory pattern, they should succeed - // even when external AWS environment variables are set, because: - // 1. Identity creation doesn't load AWS SDK config - // 2. Authentication (which does load AWS SDK config) uses LoadIsolatedAWSConfig - // 3. LoadIsolatedAWSConfig clears these variables temporarily - - t.Log("Identity creation should succeed despite conflicting AWS env vars") - t.Log("AWS SDK config loading only happens during authentication") - t.Log("Authentication uses LoadIsolatedAWSConfig which isolates environment") - - // Verify environment variables are still present after identity would be created. - assert.Equal(t, "wrong-profile", os.Getenv("AWS_PROFILE")) - assert.Equal(t, "WRONGKEY", os.Getenv("AWS_ACCESS_KEY_ID")) -}If the behavior is already covered by other integration tests, remove this one rather than keeping a minimal placeholder.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pkg/auth/cloud/aws/env.go(1 hunks)pkg/auth/cloud/aws/env_test.go(1 hunks)pkg/auth/environment_isolation_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/cloud/aws/env.go
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/cloud/aws/env_test.gopkg/auth/environment_isolation_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/auth/cloud/aws/env_test.gopkg/auth/environment_isolation_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/auth/cloud/aws/env_test.gopkg/auth/environment_isolation_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/auth/cloud/aws/env_test.gopkg/auth/environment_isolation_test.go
🧠 Learnings (2)
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*_test.go : Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Applied to files:
pkg/auth/cloud/aws/env_test.gopkg/auth/environment_isolation_test.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to tests/test_preconditions.go : Use test precondition helpers from tests/test_preconditions.go for environment checks and skip with clear reasons.
Applied to files:
pkg/auth/cloud/aws/env_test.gopkg/auth/environment_isolation_test.go
🧬 Code graph analysis (2)
pkg/auth/cloud/aws/env_test.go (1)
pkg/auth/cloud/aws/env.go (2)
WithIsolatedAWSEnv(53-85)LoadIsolatedAWSConfig(92-110)
pkg/auth/environment_isolation_test.go (4)
pkg/schema/schema_auth.go (2)
AuthConfig(4-8)IdentityVia(42-45)pkg/auth/credentials/store.go (1)
NewCredentialStore(31-33)pkg/auth/validation/validator.go (1)
NewValidator(19-21)pkg/auth/manager.go (1)
NewAuthManager(44-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/auth/cloud/aws/env_test.go (2)
12-135: Solid test coverage for environment isolation.These four tests thoroughly verify the core behavior: clearing variables during execution, restoring on success and error, handling unset variables, and partial variable scenarios. The use of
os.Unsetenvin lines 87-89 is acceptable here sinceWithIsolatedAWSEnvhandles restoration.
169-229: Good coverage and logging verification.These tests verify the list of problematic variables, logging behavior when variables are set, and no-op behavior when none are set. Well structured.
pkg/auth/environment_isolation_test.go (1)
102-150: Clean integration test with proper assertions.This test effectively verifies auth manager creation succeeds with conflicting environment variables and that those variables persist post-creation. Good coverage.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1654 +/- ##
==========================================
+ Coverage 66.10% 66.17% +0.07%
==========================================
Files 343 344 +1
Lines 38778 38817 +39
==========================================
+ Hits 25634 25688 +54
+ Misses 11155 11141 -14
+ Partials 1989 1988 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/auth/cloud/aws/env_test.go (1)
66-86: Add panic-safety coverage and make restore panic-proof.WithIsolatedAWSEnv doesn’t defer restoration; a panic would leak mutated env. Defer restore and add a test.
Apply to pkg/auth/cloud/aws/env.go (reference only):
func WithIsolatedAWSEnv(fn func() error) error { // Save original values and track which variables are being ignored. originalValues := make(map[string]string) ignoredVars := make([]string, 0) @@ - // Clear problematic variables. + // Clear problematic variables. for _, key := range problematicAWSEnvVars { os.Unsetenv(key) } - // Execute the function. - err := fn() - - // Restore original values. - for _, key := range problematicAWSEnvVars { - if value, exists := originalValues[key]; exists { - os.Setenv(key, value) - } - } - - return err + // Always restore, even on panic. + defer func() { + for _, key := range problematicAWSEnvVars { + if value, exists := originalValues[key]; exists { + _ = os.Setenv(key, value) + } else { + os.Unsetenv(key) + } + } + }() + + // Execute the function. + return fn() }Add a test here:
+func TestWithIsolatedAWSEnv_RestoresOnPanic(t *testing.T) { + t.Setenv("AWS_PROFILE", "panic-profile") + defer func() { + _ = recover() + assert.Equal(t, "panic-profile", os.Getenv("AWS_PROFILE")) + }() + _ = WithIsolatedAWSEnv(func() error { + panic("boom") + }) +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/auth/cloud/aws/env_test.go(1 hunks)pkg/auth/environment_isolation_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
🧠 Learnings (2)
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*_test.go : Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Applied to files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to tests/test_preconditions.go : Use test precondition helpers from tests/test_preconditions.go for environment checks and skip with clear reasons.
Applied to files:
pkg/auth/environment_isolation_test.gopkg/auth/cloud/aws/env_test.go
🧬 Code graph analysis (2)
pkg/auth/environment_isolation_test.go (4)
pkg/schema/schema_auth.go (2)
AuthConfig(4-8)IdentityVia(42-45)pkg/auth/credentials/store.go (1)
NewCredentialStore(31-33)pkg/auth/validation/validator.go (1)
NewValidator(19-21)pkg/auth/manager.go (1)
NewAuthManager(44-85)
pkg/auth/cloud/aws/env_test.go (2)
pkg/auth/cloud/aws/env.go (2)
WithIsolatedAWSEnv(53-85)LoadIsolatedAWSConfig(92-110)errors/errors.go (1)
ErrLoadAwsConfig(60-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/auth/cloud/aws/env_test.go (1)
230-246: Good sentinel wrapping coverage.Asserts err is wrapped with ErrLoadAwsConfig and preserves original message. LGTM.
pkg/auth/environment_isolation_test.go (3)
63-94: Nice coverage of manager creation under conflicting env vars.Validates isolation without mutating caller env. LGTM.
Also applies to: 131-146
10-13: Sort Atmos imports alphabetically within the group.Keeps linters happy and matches project style.
- "github.com/cloudposse/atmos/pkg/auth/credentials" - "github.com/cloudposse/atmos/pkg/auth/validation" - "github.com/cloudposse/atmos/pkg/schema" + "github.com/cloudposse/atmos/pkg/auth/credentials" + "github.com/cloudposse/atmos/pkg/schema" + "github.com/cloudposse/atmos/pkg/auth/validation"⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-16T15:18:00.319Z Learning: Applies to **/*.go : Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.Learnt from: samtholiya PR: cloudposse/atmos#1466 File: toolchain/http_client_test.go:3-10 Timestamp: 2025-09-10T21:17:55.273Z Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.Learnt from: aknysh PR: cloudposse/atmos#1363 File: internal/exec/template_utils.go:18-18 Timestamp: 2025-07-05T20:59:02.914Z Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
15-20: Ensure all comment lines end with periods (godot).A few comment lines lack trailing periods and will fail lint.
-// TestAuthenticationIgnoresExternalAWSEnvVars verifies that when authenticating with -// an identity (via --identity flag or default), Atmos ignores external AWS environment -// variables that could interfere with authentication. -// -// This test addresses DEV-3706: https://linear.app/cloudposse/issue/DEV-3706 +// TestAuthenticationIgnoresExternalAWSEnvVars verifies that when authenticating with. +// an identity (via --identity flag or default), Atmos ignores external AWS environment. +// variables that could interfere with authentication. +// +// This test addresses DEV-3706: https://linear.app/cloudposse/issue/DEV-3706. @@ -// Create a minimal auth configuration that would normally fail -// if AWS SDK picks up the conflicting environment variables. +// Create a minimal auth configuration that would normally fail. +// if AWS SDK picks up the conflicting environment variables. @@ -// TestAuthManagerCreationWithConflictingEnvVars verifies that creating an auth manager -// succeeds even when conflicting AWS environment variables are set. +// TestAuthManagerCreationWithConflictingEnvVars verifies that creating an auth manager. +// succeeds even when conflicting AWS environment variables are set.Also applies to: 32-34, 96-98
⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-16T15:18:00.319Z Learning: Applies to **/*.go : All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
…-avoid-authentication-issues
…ot-honor-aws_profile-to-avoid-authentication-issues
…-avoid-authentication-issues
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/auth/cloud/aws/env_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/cloud/aws/env_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use t.Skipf with a clear reason instead of t.Skip.
Mirror test file names with their corresponding implementation files (naming symmetry).
Write tests that verify behavior (inputs/outputs), avoid testing stubs or tautologies; remove always-skipped tests.
Use precondition-based test skipping helpers from tests/test_preconditions.go when external requirements are needed.
Files:
pkg/auth/cloud/aws/env_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods; treat comments as complete sentences (enforced via golangci-lint godot).
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Wrap all errors with static errors from errors/errors.go; prefer errors.Join for multiple errors; use fmt.Errorf with %w for context; check with errors.Is; never use dynamic errors directly.
Most text UI must go to stderr; only data/results go to stdout (use utils.PrintfMessageToTUI or stderr fallback).
All new configs must support Go templating via FuncMap() from internal/exec/template_funcs.go.
Prefer reusing/extending existing utilities in internal/exec and pkg over duplicating functionality.
Ensure cross-platform compatibility (Linux, macOS, Windows): prefer SDKs over shelling out; use filepath/os helpers; gate OS-specific logic with runtime.GOOS.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for each env var.
Files:
pkg/auth/cloud/aws/env_test.go
{internal,pkg}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add defer perf.Track(, "packagename.FunctionName") to all public and critical private functions, followed by a blank line.
Files:
pkg/auth/cloud/aws/env_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests for packages under pkg/ alongside implementation files.
Files:
pkg/auth/cloud/aws/env_test.go
{cmd,internal,pkg}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
{cmd,internal,pkg}/**/*.go: Send UI prompts/status/errors to stderr and data/results to stdout; do not use logging for UI.
Use structured logging for system/debug info; logging must not affect execution; follow logging levels per docs/logging.md.
Insert a blank line after defer perf.Track(...) to separate instrumentation from logic.
Files:
pkg/auth/cloud/aws/env_test.go
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure 80% minimum coverage on new/changed lines; include unit tests for new features and integration tests for CLI commands.
Files:
pkg/auth/cloud/aws/env_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-19T18:44:40.938Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-19T18:44:40.938Z
Learning: Applies to **/*_test.go : Use t.Skipf with a clear reason instead of t.Skip.
Applied to files:
pkg/auth/cloud/aws/env_test.go
🧬 Code graph analysis (1)
pkg/auth/cloud/aws/env_test.go (2)
pkg/auth/cloud/aws/env.go (2)
WithIsolatedAWSEnv(53-85)LoadIsolatedAWSConfig(92-110)errors/errors.go (1)
ErrLoadAwsConfig(60-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Analyze (go)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/auth/cloud/aws/env_test.go (8)
15-64: Solid coverage of the primary isolation behavior.The test correctly verifies clearing, preservation of AWS_REGION, and restoration of all variables.
66-86: Good error-path coverage.Verifying restoration on error is essential for avoiding environment pollution.
88-108: LGTM.Edge case for unset variables is well covered.
110-138: Nice edge-case test.Partial restoration logic is correctly verified.
140-167: Well-designed integration test.Correctly verifies region preservation and var restoration without making brittle credential assumptions.
194-214: Reasonable approach given logging constraints.The test ensures the logging code path executes without issues.
216-229: LGTM.Covers the no-op logging branch cleanly.
231-247: Well-implemented error wrapping test.Correctly verifies both sentinel error chain (errors.Is) and message inclusion.
…gha-cache * 'fix-gha-cache' of github.com:cloudposse/atmos: Isolate AWS env vars during authentication (#1654) Changes auto-committed by Conductor (#1670) Add global --chdir flag for changing working directory (#1644) Fix: Preserve exit codes from shell commands and workflows (#1660) fix: isolate golangci-lint custom build to prevent git corruption (#1666)
|
These changes were released in v1.195.0-rc.1. |
The previous fix in #1654 only isolated environment variables but the AWS SDK still loaded from ~/.aws/config and ~/.aws/credentials by default. This caused authentication failures when users had AWS_PROFILE set in their shell. The error 'failed to get shared config profile, cplive-core-gbl-identity' occurred because the SDK tried to load that profile from shared config files even though AWS_PROFILE was cleared from the environment. Solution: Use config.WithSharedConfigProfile("") to explicitly disable shared config loading in LoadIsolatedAWSConfig(). This ensures the SDK only uses credentials provided programmatically by Atmos. References: DEV-3706, #1671
* Changes auto-committed by Conductor * [autofix.ci] apply automated fixes * feat: Make AWS credentials directory configurable via provider spec Add support for configuring the AWS credentials base directory path through provider spec configuration with proper precedence handling. - Add files.base_path configuration option to provider spec - Update AWSFileManager to accept optional base_path parameter - Implement precedence: spec config > env var > default ~/.aws/atmos - Add GetFilesBasePath helper to extract path from provider spec - Update all providers (SSO, SAML) to use configured base_path - Add GetFilesDisplayPath method to AuthManager interface - Support tilde expansion for user home directory paths - Use viper.GetString for environment variable access - Update golden snapshot for auth invalid-command test This allows users to customize where AWS credential files are stored on a per-provider basis, enabling XDG-compliant paths or custom locations while maintaining backward compatibility with the default ~/.aws/atmos directory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Display actual configured paths in auth logout dry-run output Updated the `atmos auth logout` command to show the actual configured AWS files path instead of hardcoded `~/.aws/atmos/` in dry-run mode. Changes: - performIdentityLogout: Use authManager.GetFilesDisplayPath() for provider path - performProviderLogout: Use authManager.GetFilesDisplayPath() for provider path - performLogoutAll: Enhanced to show all provider paths using GetFilesDisplayPath() This ensures users see the correct path whether using the default ~/.aws/atmos or a custom path configured via spec.files.base_path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add validation for spec.files.base_path in AWS providers Added validation to ensure spec.files.base_path is properly formatted when configured in AWS auth providers (SSO and SAML). Changes: - Created ValidateFilesBasePath() function in pkg/auth/cloud/aws/spec.go - Updated SSO and SAML provider Validate() methods to call validation - Added comprehensive tests covering valid and invalid path scenarios - Validation checks for: empty/whitespace paths, invalid characters This ensures configuration errors are caught early during auth validation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Add spec.files.base_path configuration documentation Updated documentation to explain the configurable AWS files base path feature for auth logout command. Changes: - PRD: Added Configuration section with spec.files.base_path examples - PRD: Updated FR-004 to mention configurable base path - CLI docs: Added "Advanced Configuration" section with full examples - CLI docs: Documented configuration precedence and validation - CLI docs: Added example with environment variable override The documentation explains: - How to configure custom file paths in atmos.yaml - Configuration precedence (config > env var > default) - Path validation rules - Use cases for custom paths (containers, multi-user, etc.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: Add perf tracking, remove unused error, remove env var support Made several code quality improvements to auth logout implementation: 1. Performance Tracking: - Added perf.Track to executeAuthLogoutCommand in cmd/auth_logout.go - Added perf.Track to GetFilesBasePath in pkg/auth/cloud/aws/spec.go - Ensures performance monitoring for auth logout operations 2. Dead Code Removal: - Removed ErrLogoutNotSupported from errors/errors.go (never used) - All providers implement Logout, no concept of unsupported logout exists 3. Environment Variable Removal: - Removed ATMOS_AWS_FILES_BASE_PATH env var support per requirements - Only spec.files.base_path configuration is supported now - Removed viper import from pkg/auth/cloud/aws/files.go - Updated documentation to remove env var references 4. Documentation Updates: - Updated PRD to remove env var from precedence - Updated CLI docs to remove env var example and precedence - Simplified configuration to: spec config > default Changes maintain backward compatibility - default behavior unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add logout not supported vs not implemented distinction - Add ErrLogoutNotSupported (exit 0) and ErrLogoutNotImplemented (exit non-zero) - GitHub OIDC returns ErrLogoutNotSupported (no local storage to clean) - Manager treats ErrLogoutNotSupported as success - Fix duplicate identity.Logout calls with identityLogoutCalled flag - Add resolveProviderForIdentity for transitive Via chain resolution - Update LogoutProvider to find identities transitively - Add comprehensive tests (79.5% coverage in pkg/auth) * refactor: Fix N+1 provider cleanup and improve error handling in logout - Add ErrPartialLogout handling to cmd/auth_logout.go (treat as success/exit 0) - Fix N+1 provider.Logout calls in LogoutProvider using context flag - Treat keyring.ErrNotFound as success in credentialStore.Delete - Fix test map types to use types.Provider and types.Identity - Update test expectations to expect Times(1) for provider operations - Add comprehensive test for LogoutProvider with failures - Test coverage increased from 79.6% to 80.8% * fix: Add identity-scoped cleanup to preserve other identities' credentials - Add CleanupIdentity() method to AWSFileManager - Remove only specific identity's sections from shared INI files - Prevent deletion of other identities' credentials when logging out one identity - Add removeIniSection() helper to safely remove INI sections - Update aws-user identity to use CleanupIdentity instead of Cleanup This fixes the critical issue where logging out one aws-user identity would delete credentials for all identities using the same provider. * test: Update TestDelete_Flow to expect success for non-existent credentials - Delete now treats keyring.ErrNotFound as success (idempotent) - Updated test to assert.NoError instead of assert.Error - Added test for deleting already-deleted credential (idempotent behavior) * feat: Add basePath parameter to AWS setup functions - Add basePath parameter to SetupFiles() and SetEnvironmentVariables() - Update all callers to pass empty string for now (maintains default behavior) - Supports provider's configured files.base_path in future This addresses the code review comment about setup.go ignoring provider's configured files.base_path. The functions now accept basePath but callers currently pass empty string to maintain default ~/.aws/atmos behavior. Future work: Resolve actual files.base_path from provider config and pass it. * test: Add comprehensive tests for auth logout functionality - Add tests for cmd/auth_logout.go (performIdentityLogout, performProviderLogout, performLogoutAll) - Add tests for pkg/auth/cloud/aws/files.go (CleanupIdentity, removeIniSection) - Fix CleanupIdentity to use 'profile <name>' format for config file sections - Add test for 'default' identity special case handling Coverage improvements: - cmd/auth_logout.go: significantly improved from 1.77% - pkg/auth/cloud/aws/files.go: improved from 12.82% * test: Add Logout tests for AWS and GitHub providers - Add tests for SAML and SSO provider Logout methods - Add test for GitHub OIDC provider Logout (ErrLogoutNotSupported) - Verify Logout works with custom base_path configuration Coverage improvements: - pkg/auth/providers/aws/saml.go: improved Logout coverage - pkg/auth/providers/aws/sso.go: improved Logout coverage - pkg/auth/providers/github/oidc.go: improved Logout coverage * [autofix.ci] apply automated fixes * test: Add comprehensive logout tests for manager and identities - Add manager logout tests for edge cases: - Identity in chain (lines 789-803 coverage) - Identity logout returning ErrLogoutNotSupported - Identity in chain with logout failure - LogoutAll with mixed success/failure - Add identity logout tests: - AssumeRoleIdentity Logout (returns nil) - PermissionSetIdentity Logout (returns nil) - UserIdentity Logout (calls CleanupIdentity) Coverage improvements: - pkg/auth/manager.go: improved from 73.57% to near 80%+ - pkg/auth/identities/aws/assume_role.go: added Logout coverage - pkg/auth/identities/aws/permission_set.go: added Logout coverage - pkg/auth/identities/aws/user.go: added Logout coverage * [autofix.ci] apply automated fixes * Changes auto-committed by Conductor * Changes auto-committed by Conductor * Add performance instrumentation and error classification to AWS providers - Add perf.Track() instrumentation to SSO provider methods: - Validate() for validation performance tracking - Logout() for cleanup operation tracking - GetFilesDisplayPath() for path resolution tracking - Add perf.Track() instrumentation to SAML provider methods: - Logout() for cleanup operation tracking - GetFilesDisplayPath() for path resolution tracking - Update error classification in Logout methods: - Use errors.Join(ErrProviderLogout, ErrLogoutFailed, err) for proper error hierarchy - Enables better error handling and troubleshooting 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix Windows path separator compatibility in GetFilesDisplayPath tests - Normalize path separators using filepath.ToSlash() before comparison - Add path/filepath import to test files - Fixes test failures on Windows where paths use backslashes Windows was returning "~\.aws\atmos" but tests expected "~/.aws/atmos". Using filepath.ToSlash() normalizes to forward slashes for comparison. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Add performance instrumentation to SAML provider Validate method - Add perf.Track() to Validate() for performance measurement - Consistent with instrumentation pattern used in other provider methods 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Changes auto-committed by Conductor * Ignore merge conflict artifacts in gitignore - Add *.orig and *.rej to .gitignore - Remove tracked saml.go.orig file from repository These files are generated by patch/merge tools and should not be committed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Add performance instrumentation to AWS identity Logout methods - Add perf.Track() to assumeRoleIdentity.Logout() - Add perf.Track() to permissionSetIdentity.Logout() - Add perf.Track() to userIdentity.Logout() - Fix comment punctuation (all comments now end with periods) All three identity types now have consistent performance tracking for their Logout methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Add test coverage for auth components Increases test coverage across multiple auth packages: **pkg/auth/cloud/aws/files_test.go:** - Add tests for GetBaseDir() - 2 test cases - Add tests for GetDisplayPath() - 3 test cases with tilde expansion - All tests use cross-platform path normalization **pkg/auth/credentials/store_test.go:** - Add tests for SetAny() - 3 test cases - Test complex struct marshaling - Test overwriting existing values **pkg/auth/providers/aws:** - Add SAML Logout error path test (invalid base_path) - Add SSO Logout error path test (invalid base_path) **pkg/auth/identities/aws/user_test.go:** - Add test for Validate() method Coverage improvements: - pkg/auth/cloud/aws: 76.2% → 78.6% (+2.4%) - pkg/auth/providers/aws: 59.8% → 61.3% (+1.5%) - pkg/auth/credentials: 87.5% (maintained) Total: 11 new test cases added 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * Improve logout UI with concise, modern messaging Replace verbose two-step logout messages with single-line feedback: **Before:** ``` Logging out from identity: example ✓ Successfully logged out ``` **After:** ``` ✓ Logged out **example** ``` Changes: - Remove "Logging out from..." preamble messages - Show result with green checkmark (✓) on one line - Include entity name and count in success message - More modern, friendly UI matching Terraform clean pattern Examples: - Identity: `✓ Logged out **identity-name**` - Provider: `✓ Logged out provider **aws-sso** (3 identities)` - All: `✓ Logged out all 5 identities` Error messages also updated for consistency: - `✗ Failed to log out **identity-name**` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * Use PrintfMarkdownToTUI for all markdown-formatted messages Replace PrintfMessageToTUI with PrintfMarkdownToTUI for messages that use markdown formatting (**bold** syntax). This ensures: - Markdown is properly rendered instead of showing raw ** symbols - Line wrapping is handled automatically by the markdown renderer - No need for manual line breaks with \n Changes: - Error messages with **Error:** prefix - Headers like **Available identities:** - **Dry run mode:** notifications - Success messages with **bold** entity names - Warning messages like **Provider logout partially succeeded** - Browser session warning with **Note:** prefix The markdown renderer will handle formatting and line wrapping, making the output cleaner and more professional. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Show browser session warning only once using cache Reduce noise by displaying the browser session warning only on the first logout. Uses Atmos cache mechanism to track if warning has been shown, similar to how telemetry disclosure is handled. Changes: - Add BrowserSessionWarningShown field to CacheConfig - Update displayBrowserWarning() to check cache before showing - Mark warning as shown in cache after first display - Gracefully handle cache errors (warning still shows if cache fails) Behavior: - First logout: Shows warning and marks as shown in cache - Subsequent logouts: Silently skips warning - Cache location: ~/.cache/atmos/cache.yaml (respects XDG) This makes repeated logout operations much less noisy while ensuring users still see the important browser session notice at least once. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * Fix AWS env isolation to prevent shared config file loading The previous fix in #1654 only isolated environment variables but the AWS SDK still loaded from ~/.aws/config and ~/.aws/credentials by default. This caused authentication failures when users had AWS_PROFILE set in their shell. The error 'failed to get shared config profile, cplive-core-gbl-identity' occurred because the SDK tried to load that profile from shared config files even though AWS_PROFILE was cleared from the environment. Solution: Use config.WithSharedConfigProfile("") to explicitly disable shared config loading in LoadIsolatedAWSConfig(). This ensures the SDK only uses credentials provided programmatically by Atmos. References: DEV-3706, #1671 * Fix auth logout tests to expect GetIdentities and GetProviderForIdentity calls The UI improvement in ac9fe05 added code to count identities per provider for better logout messages. This requires calling GetIdentities() and GetProviderForIdentity() in performProviderLogout(). Updated mock expectations in tests to match the actual code flow: - Added GetIdentities() mock for provider logout tests - Added GetProviderForIdentity() mock to determine which identities use the provider - Added GetIdentities() mock for successful logout all (to show count) - Correctly excluded GetIdentities() from partial logout all (returns early) * Fix AWS setup tests using homedir.Reset() to clear cache The tests were failing in CI because the homedir package caches the home directory value. When tests use t.Setenv("HOME"), the cached value isn't updated. Proper fix: Call homedir.Reset() after setting HOME to clear the cache. This forces homedir.Dir() to re-detect the home directory from the updated environment variable. This approach: - Uses the intended homedir cache management API - Tests the actual default path behavior (empty basePath parameter) - Is more realistic than passing explicit paths - Matches the pattern documented in PR #1673 * Changes auto-committed by Conductor * Replace deprecated github.com/golang/mock with go.uber.org/mock - Update all mock files and test files to use go.uber.org/mock/gomock - Update go.mod and go.sum with new dependency - Fixes CI compilation errors from deprecated gomock package - Note: Skipping golangci-lint for pre-existing issues in auth code * Add perf instrumentation and fix import ordering - Add perf tracking to Cleanup, CleanupIdentity, and CleanupAll in pkg/auth/cloud/aws/files.go - Add context parameter to CleanupIdentity signature and update all callers - Fix import ordering in validate_schema_test.go and version_test.go per project guidelines - Fix missing period in version_test.go comment - Add missing context import to files_test.go - All tests passing * Add blog post for atmos auth shell with corrected problem statement - Reposition problem statement to focus on secure multi-identity workflows - Emphasize session scoping, credential isolation, and security benefits - Compare with AWS Vault's approach to credential management - Add clear guidance on when to use 'auth shell' vs 'auth env' - Include real-world workflows and multi-identity scenarios - Fix all documentation links to use correct URL patterns * Fix missing periods in blog post list items - Add terminal punctuation to all list items in 'How It Works' section - Ensures consistent punctuation throughout the document * Remove auth shell blog post and add auth logout blog post - Remove auth shell blog post (doesn't belong in logout PR) - Add auth logout blog post focusing on general cloud tooling problem - Position logout as solving credential cleanup difficulty across all providers - Emphasize Atmos Auth has only been out for less than a week - Frame problem as 'cloud tooling doesn't make logout easy' not 'Atmos lacked logout' * Remove duplicate auth logout blog post - Already have 2025-10-17-auth-logout-feature.md - Shouldn't have created a duplicate * Update auth logout blog post problem statement - Reframe problem: cloud tooling doesn't make logout easy - Focus on credential sprawl across AWS, Azure, GCP - Emphasize manual cleanup burden and security risks - Position Atmos logout as solving general cloud tooling gap - Remove implication that Atmos previously lacked logout * Update auth logout documentation with improved problem statement - Add context about cloud tooling not making logout easy - Mention credential sprawl across AWS, Azure, GCP - Frame logout as making cleanup explicit and comprehensive - Maintain existing detailed documentation structure * Move problem statement to dedicated section in logout docs - Keep intro concise and focused on what the command does - Add 'The Problem' section below intro - Explains credential sprawl and lack of easy logout in cloud tooling - Positions Atmos logout as the solution * [autofix.ci] apply automated fixes * Fix AWS auth tests to use forked homedir package - Tests were calling Reset() on mitchellh/go-homedir - Production code uses our forked pkg/config/homedir - This caused cache to not be cleared properly between test runs - Update both setup_test.go and files_test.go to use our fork - Tests now pass reliably when run multiple times - Add comprehensive Reset() tests to homedir package * Add lint rule to forbid mitchellh/go-homedir imports - Add forbidigo rule to prevent importing github.com/mitchellh/go-homedir - Direct developers to use our forked pkg/config/homedir instead - Prevents future test flakiness from using wrong homedir package - Config verified with golangci-lint config verify * Fix blog post formatting and homedir test flakiness Blog post fixes (2025-10-17-auth-logout-feature.md): - Add shell language specifiers to 7 code blocks (lines 44, 71, 92, 113, 132, 153, 213) - Add missing periods to list items (lines 182, 311, 320) Homedir test fixes (pkg/config/homedir/homedir_test.go): - Add Reset() calls to TestDir and TestExpand to prevent cross-test interference - Fix TestExpand expected path construction (remove trailing separator) - Tests now pass reliably when run multiple times --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
what
pkg/auth/cloud/aws/env.go) to manage the isolation of problematic AWS environment variables during authentication.WithIsolatedAWSEnv()function that temporarily clears a predefined list of AWS environment variables, executes a provided function, and then restores the original values.LoadIsolatedAWSConfig()which wraps AWS SDK'sconfig.LoadDefaultConfig()and utilizesWithIsolatedAWSEnv()to ensure environment variables do not interfere with AWS config loading.LoadIsolatedAWSConfig()instead ofconfig.LoadDefaultConfig()when initializing AWS SDK clients. This includes:pkg/auth/identities/aws/assume_role.gopkg/auth/identities/aws/permission_set.gopkg/auth/identities/aws/user.gopkg/auth/providers/aws/saml.gopkg/auth/providers/aws/sso.gowhy
AWS_PROFILE,AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY,AWS_SESSION_TOKEN,AWS_CONFIG_FILE,AWS_SHARED_CREDENTIALS_FILE) could interfere with Atmos's internal AWS authentication mechanisms, particularly when using AWS IAM Identity Center (SSO) or assuming roles. This often led to authentication failures or unexpected behavior.internal/aws_utils/aws_utils.gofile, which is used in contexts where external environment variables are expected to be honored (e.g., Terraform backend configuration), continues to useconfig.LoadDefaultConfig()to avoid breaking existing functionality.references
Summary by CodeRabbit
New Features
Tests