Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pop explicit credentials when switching profiles #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joguSD
Copy link
Contributor

@joguSD joguSD commented Aug 2, 2017

The CLI will prefer explicit AWS credentials in the environment variables thus ignoring the profile set by the shell. If you manually invoke a profile using the shell this will ensure that any explicit credentials are removed from the shell's environment.

Fixes #166

Copy link
Member

@jamesls jamesls left a comment

Choose a reason for hiding this comment

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

I'm not sure this is the right fix. We should still be able to pull creds from env var if no explicit profile is given (aws-shell with no args). Instead I would just expect us to explicitly set the profile when invoking the CLI command.

@joguSD
Copy link
Contributor Author

joguSD commented Aug 7, 2017

@jamesls This will still allow us to pull creds from the env vars if no explicit profile is set. The code in the PR is only executed if -p is passed or .profile <profile> is invoked. I think having implicit credentials override the explicitly set profile is counter intuitive.

I had thought of adding a --profile <profile> to the command executed if a profile is present, but I think it's fragile and could potentially have a lot edge cases. I think the shell should avoid manipulating the command executed as much as possible.

@kyleknap
Copy link

@joguSD I also don't think popping the environment variables is the right thing to do either. I actually think we should not be setting the AWS_DEFAULT_PROFILE, which is what the current implementation does as well. In general environment variables are the user's space. We should not be manipulating them silently as it can lead to unexpected results.

The best option in general would be to set the profile on the session. However, we do not have access to that as the command is ran in a subprocess. So for now the best you probably would be able to do is set --profile when the command is ran. But like you said, there will probably be a decent amount of edge cases with it.

@joguSD
Copy link
Contributor Author

joguSD commented Aug 10, 2017

@kyleknap I can see where you're coming from about the environment variables thing, but I'm not sure if it's entirely relevant in the current state of variable support of the shell. Upon entering the aws-shell we copy the parent process environment and have an internal copy that we pass to sub-processes. Once this copy is made it can no longer be manipulated by the user within the shell. We do not support export or source currently. If this was a CLI feature I would totally agree, but I'm not sure if the same logic applies to the shell's internal environment variables. This is a shell feature. You are setting the profile for the shell, and if you were to do that in bash or zsh you would simply export the profile. This is an alias for that behavior in our shell.

I really don't think the behavior of .profile should be to append --profile <profile> to cli commands. This would require us to check every cli command before it's executed for the presence of a profile and only in the case that it's not passed should it be appended. There may be other situations where simply adding cmd + ' --profile {profile}' will not be sufficient. I also don't think the command the user types and the command that is executed should ever differ. This will complicate the command history and the .edit functionality.

@JordonPhillips JordonPhillips removed their request for review May 13, 2020 22:23
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.

3 participants