auth/aws: fix panic in IAM-based login when client config doesn't exist #23366
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
builtin/credential/aws/path_login.go
Outdated
| return "", nil, nil, logical.ErrorResponse("error getting configuration"), nil | ||
| return "", nil, nil, nil, fmt.Errorf("error getting configuration: %w", err) | ||
| } | ||
| if config == nil { |
There was a problem hiding this comment.
This is a behavior change to not allow IAM-based login when the client configuration does not exist. This was permitted prior to the change that introduced the panic (for example, in 1.14.x).
I could use thoughts on whether this is a change we want to make.
There was a problem hiding this comment.
I think this change make sense. I am not sure what credentials are used to make API calls if the client config does not exist.
There was a problem hiding this comment.
I'd have to look at the code, but without allowing an empty client configuration you're effectively disallowing instance profiles from being used to provide AWS credentials, right? The AWS Go SDK will fall back to using them if you aren't overriding how the default credentials chain is managed. The Vault docs also suggest that not providing a client configuration is supported:
If static credentials are not provided using this endpoint, then the credentials will be retrieved from the environment variables AWS_ACCESS_KEY, AWS_SECRET_KEY and AWS_REGION respectively. If the credentials are still not found and if the method is configured on an EC2 instance with metadata querying capabilities, the credentials are fetched automatically.
There was a problem hiding this comment.
Thanks for the details, @protochron! I'm looking a little closer at how the client is configured in the case where the client config hasn't been written. I don't want to fix this panic and break more users with the behavior change of requiring it.
There was a problem hiding this comment.
@protochron is right that the default credential chain will be used if the client configuration isn't written. This means the credentials will be source from env vars, instance profiles, etc.
I think it would be less disruptive to maintain the behavior which doesn't require the client config to be written. If we changed the behavior, users would need to write an empty config similar to the workaround in the known issue included in this PR.
I'm going to change this so that the panic is fixed and the behavior of not requiring the client config is maintained.
There was a problem hiding this comment.
@austingebauer that sounds perfect. It wouldn't necessarily be the worst thing to write an empty config if there was any other precedent in Vault for it, but I think people would be surprised by that being necessary. I certainly was!
Thanks again for deep diving on this issue.
There was a problem hiding this comment.
Yep, I'd rather avoid telling users that they need to write an empty config after upgrading to 1.15.0+ for their logins to continue working. We will maintain the current behavior and fix the panic. I've made those changes in 34d8673.
No problem, sorry for any trouble this caused!
|
Build Results: |
|
CI Results: |
…st (#23366) * auth/aws: fix panic in IAM-based login when client config doesn't exist * add changelog * adds known issue for 1.15.0 * fixes up known issue with workaround * fix link * maintain behavior of client config not needing to exist for IAM login * update changelog
This PR fixes a panic in AWS auth method for IAM-based login when the client config doesn't exist. The panic exists in Vault 1.15 by way of #21960. I've added a test that would cause the panic without these changes.
Steps taken to reproduce:
Fixes: #23361