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

Make the exec command default to a login shell #546

Closed

Conversation

willejs
Copy link
Contributor

@willejs willejs commented Mar 25, 2020

Currently when you run aws-vault exec <profile> it runs $SHELL and puts you into a non login shell. This does not execute your shells login profile so you end up losing anything nice you have in your login profile.

Instead I think exec should default to executing a login shell. As far as I am aware all shells support -l, so this should be backwards compatible, and i don't envision any issues. This attempts to close out issue #406 too.

@mtibben
Copy link
Member

mtibben commented Apr 3, 2020

How does this behave on Windows ?

@frezbo
Copy link
Contributor

frezbo commented Apr 3, 2020

This can be easily fixed by running aws-vault exec <profile> -- bash This provides a login shell as per the system config.

@willejs
Copy link
Contributor Author

willejs commented Apr 3, 2020

This can be easily fixed by running aws-vault exec <profile> -- bash This provides a login shell as per the system config.

You can do this, but when you are using aws-vault exec often (when your session times out) its quite painful. Also, im not sure why you would ever want to exec into a non login shell? So i think makes sense to default to a login shell?

How does this behave on Windows ?

Good catch! I dont know to be honest, i didn't think of that! Is there anyone who can run $SHELL -l on windows? Not sure what that even does!
ahh, i am not sure about this?

@willejs
Copy link
Contributor Author

willejs commented Apr 14, 2020

How does this behave on Windows ?

I finally managed to test this with a vagrant box. By default it executes cmd.exe, which does not like -l. I am wondering if instead this could be set to an env var? or i could do an if windows, but I am not sure how that would work.

@willejs willejs force-pushed the default-exec-to-login-shell branch from 490664d to 660c5e3 Compare April 15, 2020 08:58
@mtibben
Copy link
Member

mtibben commented Apr 15, 2020

Rather than blacklisting windows, could you whitelist shells that accept the -l flag? I believe you can run bash/zsh just fine on Windows these days and it would still be useful on windows when those shells are running

@mtibben mtibben closed this in 38262fd Apr 18, 2020
@segevfiner
Copy link
Contributor

I didn't test yet, but wouldn't this cause a second execution of the shell profile under the same environment if aws-vault is executed from a login shell or a shell that has inherited the login env? This often results in doubled entries in PATH and other unwanted side effects and often caused many problems for me in other scenarios where such a situation happened.

@mtibben
Copy link
Member

mtibben commented Apr 21, 2020

@segevfiner aws-vault by default won't execute inside it's own shell, which it determines by checking if AWS_VAULT is set.

Can you be more specific about a scenario where this might cause issues?

@segevfiner
Copy link
Contributor

I'm running inside a login shell or a shell that has inherited the login shell environment as is the norm in any terminal emulator or IDE embedded terminal. I execute aws-vault inside that, which will request to start another login shell under that shell that is already a login shell or has inherited the login shell environment.

This leads to an environment that has run the login shell script twice, leading to unexpected side effects as that script expects to be called once per session (Unlike the shell rc script, e.g. bashrc. Such side effects include doubled PATH environment variables, wiping environment variables that were expected to be inherited, causing stuff like ssh-agent to be started a second time for profiles that do so, etc.

This assumes that I understood what this PR does. I didn't upgrade seeing this in the change log to avoid issues that I won't have time to work to fix ATM, at least before figuring this out here.

@mtibben
Copy link
Member

mtibben commented Apr 30, 2020

Yes I see what you're saying @segevfiner. Hmmm maybe I'll revert this for now, aim for v6 to work out how to do this better

@mtibben mtibben reopened this Apr 30, 2020
mtibben added a commit that referenced this pull request Apr 30, 2020
@mtibben mtibben added this to the v6 milestone Apr 30, 2020
@mtibben mtibben closed this in 481e413 Apr 30, 2020
@mtibben mtibben reopened this May 1, 2020
@mtibben
Copy link
Member

mtibben commented May 1, 2020

Hey @willejs I've just been playing around with this.

I don't think a login shell is required. aws-vault already sends through the entire environment of the current shell to the new shell.

What will not come across to the new shell are local variables and functions, which may include your PS1 prompt.

If that is what you're noticing, there is a simple solution. Define any local variables and functions like the prompt functions and PS1 in the .bashrc or .zshrc which gets executed every time a new shell is started.

Does that solve your problem?

The problem with the login shell is as @segevfiner described - by running the shell profile scripts again (which are often additive) you can put your shell in a weird state. If a login shell really is required, we probably shouldn't send the entire environment through to the new shell

@mtibben
Copy link
Member

mtibben commented May 2, 2020

Added docs here be6e1b6. Does this solve your problem @willejs ? (if not #406 looks like a better way forward)

@mtibben mtibben closed this May 2, 2020
@mtibben mtibben removed this from the v6 milestone May 2, 2020
@segevfiner
Copy link
Contributor

It is as described by @mtibben.

Anything that should run only once should be in one of the profile script .bash_profile/.profile/.zprofile. But there is weirdness on when and which they get executed on different OS, so it takes some trial and error sometimes to figure out the behavior on the exact OS and DM, etc you are using. For example on Ubuntu by default the DM only sources .profile, but SSH with ZSH as default will source .zprofile. So I had .zprofile source .profile with appropriate compatibility.

The .bashrc/.zshrc should include anything that needs to be set on every shell, including every subshell and will be executed multiple times inheriting variables from previous executions. So this is basically anything else that isn't inherited by subshells.

Also note that many many tutorials and instructions get this wrong too.

@frezbo
Copy link
Contributor

frezbo commented May 3, 2020

@segevfiner You sent me down a rabbit hole and made me learn something new. Thanks 👍. Based on my new learning I started using pathmunge function from /etc/profile to add in new PATH, I was getting a lot of duplicate path entries (it never caused a problem though).

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.

4 participants