Skip to content

Conversation

xilu0
Copy link

@xilu0 xilu0 commented Sep 16, 2025

Related: #123

This commit introduces two main changes to the config loading logic to prevent infinite loops.

First, it adds loop detection for the source_profile chain. This prevents aws-vault from crashing when a profile has a circular dependency on another profile via source_profile.

Second, it removes the implicit inheritance of the default profile for all other profiles. This aligns aws-vault's behavior with the AWS CLI and prevents unexpected loops when the default profile has a source_profile set. This was causing a bug where aws-vault would fail to load a valid AWS config.

log:
error-debug.log

@xilu0 xilu0 requested a review from mbevc1 as a code owner September 16, 2025 11:25
@xilu0 xilu0 force-pushed the main branch 2 times, most recently from 3d26182 to f84222b Compare September 16, 2025 14:48
@mbevc1 mbevc1 changed the title This commit introduces two main changes to the config loading logic t… fix(config): prevent profile inheritance loops Sep 16, 2025
@mbevc1
Copy link

mbevc1 commented Sep 16, 2025

Seems like your change is breaking tests:

--- FAIL: TestIncludeProfile (0.00s)
panic: assignment to entry in nil map [recovered, repanicked]

Please ignore PR checks for now - seems perms issue when running on a fork branch 🤦

@mbevc1
Copy link

mbevc1 commented Sep 16, 2025

Would you mind rebasing to main to test PR checks GHA as well please?

…o prevent infinite loops.

First, it adds loop detection for the source_profile chain. This prevents aws-vault from crashing when a profile has a circular dependency on another profile via source_profile.

Second, it removes the implicit inheritance of the default profile for all other profiles. This aligns aws-vault's behavior with the AWS CLI and prevents unexpected loops when the default profile has a source_profile set. This was causing a bug where aws-vault would fail to load a valid AWS config.
Replace direct struct instantiation with NewConfigLoader constructor to properly initialize the sourceChain map.
This prevents panics when GetProfileConfig tries to assign to the nil map.
@xilu0
Copy link
Author

xilu0 commented Sep 17, 2025

Hi @mbevc1, I've rebased onto ByteNess/aws-vault/main.
All tests are now passing. The issue was that several test cases were creating ConfigLoader instances using struct literals instead of the NewConfigLoader constructor function.
This left the sourceChain map uninitialized (nil), causing panics when the GetProfileConfig method tried to assign to the map.
Summary of fixes made:

  1. Root cause: Tests were creating ConfigLoader using &vault.ConfigLoader{File: configFile} instead of vault.NewConfigLoader(), leaving the sourceChain map nil.
  2. Files fixed:
    - vault/config_test.go: Fixed 7 instances across multiple test functions
    - vault/vault_test.go: Fixed 3 instances in test functions
  3. Solution: Replaced all direct struct literal instantiations with proper constructor calls:
    // Before (causing nil map panic)
    configLoader := &vault.ConfigLoader{File: configFile}
    // After (properly initialized)
    configLoader := vault.NewConfigLoader(vault.ProfileConfig{}, configFile, "")

The NewConfigLoader constructor properly initializes the sourceChain map with make(map[string]bool), preventing the nil map assignment panic that was occurring in vault/config.go:519.

@mbevc1
Copy link

mbevc1 commented Sep 17, 2025

Thanks for your contribution! I'd like to check the logic here - wouldn't there be some options we'd like to inherit from default, e.g. sts_regional_endpoints or mfa_serial?

Side note, would you mind rebasing again, I have new GHA for conventional commit in, which should work better 🤞

Comment on lines -408 to -412
} else if profileName != defaultSectionName {
err := cl.populateFromConfigFile(config, defaultSectionName)
if err != nil {
return err
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we want to inherit default profile? At least in all non-chained profiles? 🤔

@mbevc1
Copy link

mbevc1 commented Sep 29, 2025

@xilu0 PR checks should be fixed if you rebase to main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants