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

Upstart export is no executing command with rbenv path of owned user #443

Closed
wireframe opened this Issue Apr 28, 2014 · 23 comments

Comments

Projects
None yet
9 participants
@wireframe

wireframe commented Apr 28, 2014

Upgrading to latest version of foreman broke the generated upstart configuration files and upstart is reporting the following error now when trying to start the application server:

/bin/sh: 1: exec: bundle: not found

My Procfile:

web: bundle exec puma -p $PORT

My Environment:

  • vagrant VM
  • ubuntu 12.04
  • rbenv

This regression introduced by commit c039f37 as part of #438. Using setuid <%= user %> does not seem to inherit the proper user PATH required for rbenv to execute the target binary. The previous version used exec su - <%= user %> which properly inherited rbenv on the path.

@ddollar

This comment has been minimized.

Show comment
Hide comment
@ddollar

ddollar Apr 29, 2014

Owner

What is the proper way to get the user's login scripts to run in upstart?

/cc @phemmer

Owner

ddollar commented Apr 29, 2014

What is the proper way to get the user's login scripts to run in upstart?

/cc @phemmer

@phemmer

This comment has been minimized.

Show comment
Hide comment
@phemmer

phemmer Apr 29, 2014

Contributor

You would need to specify the command to run (in the Procfile) as something like /bin/bash -l -c 'command here', or use an absolute path to bundle. Or set PATH in the .env file.

The way my group (at work) does this is by setting PATH in the .env file. It's simple & clean.
 

Using a shell from upstart is not supported as it is a security vulnerability. Some security compliance audits will even flag such an issue and require that it be fixed.

This is even covered in the NSA document Secure Configuration of Red Hat Enterprise Linux 5 - Section 2.3.1.4 "Block Shell and Login Access for Non-Root System Accounts"

There are several other supporters of this as well:
http://www.cyberciti.biz/tips/howto-linux-shell-restricting-access.html
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=330882
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=274229

Contributor

phemmer commented Apr 29, 2014

You would need to specify the command to run (in the Procfile) as something like /bin/bash -l -c 'command here', or use an absolute path to bundle. Or set PATH in the .env file.

The way my group (at work) does this is by setting PATH in the .env file. It's simple & clean.
 

Using a shell from upstart is not supported as it is a security vulnerability. Some security compliance audits will even flag such an issue and require that it be fixed.

This is even covered in the NSA document Secure Configuration of Red Hat Enterprise Linux 5 - Section 2.3.1.4 "Block Shell and Login Access for Non-Root System Accounts"

There are several other supporters of this as well:
http://www.cyberciti.biz/tips/howto-linux-shell-restricting-access.html
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=330882
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=274229

@technicool

This comment has been minimized.

Show comment
Hide comment
@technicool

technicool Apr 29, 2014

👍 This appears to still work with foreman 0.63.0, but not 0.66.0 I haven't tracked down which revision it changed in, but I agree that the existing functionality appears broken for upstart exporting. However, it appears that the newly created scripts may be more "proper", so a patch to load the user's profile may be in order.

technicool commented Apr 29, 2014

👍 This appears to still work with foreman 0.63.0, but not 0.66.0 I haven't tracked down which revision it changed in, but I agree that the existing functionality appears broken for upstart exporting. However, it appears that the newly created scripts may be more "proper", so a patch to load the user's profile may be in order.

@phemmer

This comment has been minimized.

Show comment
Hide comment
@phemmer

phemmer Apr 29, 2014

Contributor

This can't be reasonably supported.

Let's assume the user's shell is /bin/bash, and you have upstart wrap the command in bash -l. That would work, but then what if the user's shell isn't bash?
You can't use /bin/sh -l either as -l isn't guaranteed to be a supported argument to sh (it's not defined in POSIX).
But you also can't use su or other mechanism to launch the user's shell, whatever it may be, because the user might not even have a shell (as I mentioned earlier, giving system accounts a shell is a security vulnerability).
 

You are right that this is a significant change, and I did forget about this possibility when creating that pull request.
I'm not sure what foreman's policy is on backwards incompatible changes, but I've seen such changes made frequently. However this is exactly what utilities such as bundler are for. They allow the user to upgrade the gem once they've fixed their code to be compatible with it.

Contributor

phemmer commented Apr 29, 2014

This can't be reasonably supported.

Let's assume the user's shell is /bin/bash, and you have upstart wrap the command in bash -l. That would work, but then what if the user's shell isn't bash?
You can't use /bin/sh -l either as -l isn't guaranteed to be a supported argument to sh (it's not defined in POSIX).
But you also can't use su or other mechanism to launch the user's shell, whatever it may be, because the user might not even have a shell (as I mentioned earlier, giving system accounts a shell is a security vulnerability).
 

You are right that this is a significant change, and I did forget about this possibility when creating that pull request.
I'm not sure what foreman's policy is on backwards incompatible changes, but I've seen such changes made frequently. However this is exactly what utilities such as bundler are for. They allow the user to upgrade the gem once they've fixed their code to be compatible with it.

@technicool

This comment has been minimized.

Show comment
Hide comment
@technicool

technicool Apr 29, 2014

The officially documented blog post in the Readme lists the following Procfile:

web:    bundle exec thin start -p $PORT
worker: bundle exec rake resque:work QUEUE=*
clock:  bundle exec rake resque:scheduler

Now, if we call foreman export upstart to make it run in production, this will break on 100% of machines. We either need an updated documentation, or need to find a way where exporting this baseline script could possibly work without undocumented tricks.

technicool commented Apr 29, 2014

The officially documented blog post in the Readme lists the following Procfile:

web:    bundle exec thin start -p $PORT
worker: bundle exec rake resque:work QUEUE=*
clock:  bundle exec rake resque:scheduler

Now, if we call foreman export upstart to make it run in production, this will break on 100% of machines. We either need an updated documentation, or need to find a way where exporting this baseline script could possibly work without undocumented tricks.

@wireframe

This comment has been minimized.

Show comment
Hide comment
@wireframe

wireframe Apr 29, 2014

just my 2 cents...

I'm totally cool with providing a more sensible and secure default, and if there is a recommended way to update my environment to run with this new recommendation, I'm all for it (I'm not a fulltime devop).

That being said, my setup seems pretty standard (rbenv + upstart + appserver/puma) and foreman has been awesome at getting this stack up and running quickly/easily. My hope that a solution can be provided to balance ease of use for developers and security (hopefully without breaking the API contract already in place :)

wireframe commented Apr 29, 2014

just my 2 cents...

I'm totally cool with providing a more sensible and secure default, and if there is a recommended way to update my environment to run with this new recommendation, I'm all for it (I'm not a fulltime devop).

That being said, my setup seems pretty standard (rbenv + upstart + appserver/puma) and foreman has been awesome at getting this stack up and running quickly/easily. My hope that a solution can be provided to balance ease of use for developers and security (hopefully without breaking the API contract already in place :)

@phemmer

This comment has been minimized.

Show comment
Hide comment
@phemmer

phemmer Apr 29, 2014

Contributor

@technicool That Procfile would work perfectly fine. Just put PATH in the .env file.

Contributor

phemmer commented Apr 29, 2014

@technicool That Procfile would work perfectly fine. Just put PATH in the .env file.

@wireframe

This comment has been minimized.

Show comment
Hide comment
@wireframe

wireframe Apr 29, 2014

@technicool see issue description for source of regression:

This regression introduced by commit c039f37 as part of #438.

wireframe commented Apr 29, 2014

@technicool see issue description for source of regression:

This regression introduced by commit c039f37 as part of #438.

@wireframe

This comment has been minimized.

Show comment
Hide comment
@wireframe

wireframe Apr 29, 2014

Just put PATH in the .env file.

@phemmer can you clarify? set the PATH to what? hardcode to the path of rbenv? the PATH for most user accounts is not that simple. ex:

PATH=/home/vagrant/.rbenv/shims:/home/vagrant/.rbenv/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/opt/vagrant_ruby/bin

a few issues i see with that approach:

  • hardcoding user path's in .env breaks running the application between vagrant and host OS user accounts.
  • hardcoding /usr/bin vs /usr/local/bin paths for OSX vs vagrant.

Is there an elegant way to solve these issues?

wireframe commented Apr 29, 2014

Just put PATH in the .env file.

@phemmer can you clarify? set the PATH to what? hardcode to the path of rbenv? the PATH for most user accounts is not that simple. ex:

PATH=/home/vagrant/.rbenv/shims:/home/vagrant/.rbenv/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/opt/vagrant_ruby/bin

a few issues i see with that approach:

  • hardcoding user path's in .env breaks running the application between vagrant and host OS user accounts.
  • hardcoding /usr/bin vs /usr/local/bin paths for OSX vs vagrant.

Is there an elegant way to solve these issues?

@technicool

This comment has been minimized.

Show comment
Hide comment
@technicool

technicool Apr 29, 2014

@wireframe I agree that PATH's being hardcoded into the repo would be a problem when deploying across different environments.

I imagine a bunch of people who do a bundle update soon are going to be running into this issue, so we should come up with a way to pull this easily.

technicool commented Apr 29, 2014

@wireframe I agree that PATH's being hardcoded into the repo would be a problem when deploying across different environments.

I imagine a bunch of people who do a bundle update soon are going to be running into this issue, so we should come up with a way to pull this easily.

@technicool

This comment has been minimized.

Show comment
Hide comment
@technicool

technicool Apr 29, 2014

Would it be possible to prefix the latest version with /bin/sh -l (I know, not posix, but works many places) or /bin/bash -l, since that will fix most users to the current behavior.

Then, add a --no-shell option that will disable this behavior for those who wish to dig deeper and ensure their systems are as secure/lean as possible.

technicool commented Apr 29, 2014

Would it be possible to prefix the latest version with /bin/sh -l (I know, not posix, but works many places) or /bin/bash -l, since that will fix most users to the current behavior.

Then, add a --no-shell option that will disable this behavior for those who wish to dig deeper and ensure their systems are as secure/lean as possible.

@phemmer

This comment has been minimized.

Show comment
Hide comment
@phemmer

phemmer Apr 29, 2014

Contributor

can you clarify? set the PATH to what? hardcode to the path of rbenv?

The path to bundle. Since the Procfile is invoking bundle, that's all it needs to find. Bundler takes care of setting up the rest of the environment.

hardcoding user path's in .env breaks running the application between vagrant and host OS user accounts.

Part of the standard way of deploying bundler apps is to run bundle install --deployment on the same architecture as your production servers. It is at this time that the .env file should be defined with the proper $PATH.

However now we're getting into deployment procedures, and while the official bundler way is by far the most supported route, it is open to the user.


While I personally think the proper solution is to use bundler to hold the gem at a supported version, if this really must be supported, I think the least evil solution would be to pull the $PATH from the environment of foreman export, and put that in the upstart file (unless PATH is explicitly defined in .env).
This would be a simple change, but I will defer to @ddollar

Contributor

phemmer commented Apr 29, 2014

can you clarify? set the PATH to what? hardcode to the path of rbenv?

The path to bundle. Since the Procfile is invoking bundle, that's all it needs to find. Bundler takes care of setting up the rest of the environment.

hardcoding user path's in .env breaks running the application between vagrant and host OS user accounts.

Part of the standard way of deploying bundler apps is to run bundle install --deployment on the same architecture as your production servers. It is at this time that the .env file should be defined with the proper $PATH.

However now we're getting into deployment procedures, and while the official bundler way is by far the most supported route, it is open to the user.


While I personally think the proper solution is to use bundler to hold the gem at a supported version, if this really must be supported, I think the least evil solution would be to pull the $PATH from the environment of foreman export, and put that in the upstart file (unless PATH is explicitly defined in .env).
This would be a simple change, but I will defer to @ddollar

@ddollar

This comment has been minimized.

Show comment
Hide comment
@ddollar

ddollar May 2, 2014

Owner

Am I correct in assuming that the core of the issue is that the new upstart scripts are not loading the login scripts for the app user?

Owner

ddollar commented May 2, 2014

Am I correct in assuming that the core of the issue is that the new upstart scripts are not loading the login scripts for the app user?

@phemmer

This comment has been minimized.

Show comment
Hide comment
@phemmer

phemmer May 2, 2014

Contributor

That is the departure from the previous behavior yes.
I would say the core of the issue is that it is not using the same ruby as was in use during the foreman export.

Contributor

phemmer commented May 2, 2014

That is the departure from the previous behavior yes.
I would say the core of the issue is that it is not using the same ruby as was in use during the foreman export.

@ddollar

This comment has been minimized.

Show comment
Hide comment
@ddollar

ddollar May 2, 2014

Owner

After some thinking on this issue I believe that having the app rely on behavior from the executing user's login scripts is undesirable. If your app requires extra environment variables to run they should be included explicitly in an .env file or wrapper scripts in your app. I typically create a bin/web script to handle things like this.

I realize that this is a breaking change and I apologize for that but I think it will be better in the long run.

Owner

ddollar commented May 2, 2014

After some thinking on this issue I believe that having the app rely on behavior from the executing user's login scripts is undesirable. If your app requires extra environment variables to run they should be included explicitly in an .env file or wrapper scripts in your app. I typically create a bin/web script to handle things like this.

I realize that this is a breaking change and I apologize for that but I think it will be better in the long run.

@ddollar ddollar closed this May 2, 2014

@wireframe

This comment has been minimized.

Show comment
Hide comment
@wireframe

wireframe May 2, 2014

@ddollar just my 2 cents, but i would highly recommend adding a readme or FAQ entry somewhere to help users mitigate this API change.

I will personally be staying with v0.63 until there is a clear and simple way to play nicely with rbenv and upstart.

sidenote: I'm also interested to see how systemd approaches this issue now that upstart is on it's way out the door.
http://www.zdnet.com/after-linux-civil-war-ubuntu-to-adopt-systemd-7000026373/

wireframe commented May 2, 2014

@ddollar just my 2 cents, but i would highly recommend adding a readme or FAQ entry somewhere to help users mitigate this API change.

I will personally be staying with v0.63 until there is a clear and simple way to play nicely with rbenv and upstart.

sidenote: I'm also interested to see how systemd approaches this issue now that upstart is on it's way out the door.
http://www.zdnet.com/after-linux-civil-war-ubuntu-to-adopt-systemd-7000026373/

@vinnividivicci

This comment has been minimized.

Show comment
Hide comment
@vinnividivicci

vinnividivicci May 5, 2014

I'm with @wireframe on that one, had to revert back to 0.63.0

vinnividivicci commented May 5, 2014

I'm with @wireframe on that one, had to revert back to 0.63.0

@Floppy

This comment has been minimized.

Show comment
Hide comment
@Floppy

Floppy May 8, 2014

We've resolved this by getting our deploy scripts to write the bundle path to a file, then using foreman's -e option to tell it to look at both the standard .env file and our new path file. Seems to be working.

Floppy commented May 8, 2014

We've resolved this by getting our deploy scripts to write the bundle path to a file, then using foreman's -e option to tell it to look at both the standard .env file and our new path file. Seems to be working.

@dominicsayers

This comment has been minimized.

Show comment
Hide comment
@dominicsayers

dominicsayers May 22, 2014

I've had to change my deployment scripts to add HOME, SHELL and PATH to my .env file which was a pain (because it was unexpected), but having done it I now prefer the result.

The running production machine is now cleaner and easier to manage.

I'd add a vote to update the documentation to make it clear that much (all?) login processing is now bypassed so the user is responsible for setting up the environment.

dominicsayers commented May 22, 2014

I've had to change my deployment scripts to add HOME, SHELL and PATH to my .env file which was a pain (because it was unexpected), but having done it I now prefer the result.

The running production machine is now cleaner and easier to manage.

I'd add a vote to update the documentation to make it clear that much (all?) login processing is now bypassed so the user is responsible for setting up the environment.

@technicool

This comment has been minimized.

Show comment
Hide comment
@technicool

technicool May 22, 2014

I added a note in the wiki. Hopefully that helps out.

https://github.com/ddollar/foreman/wiki/Exporting-for-production

technicool commented May 22, 2014

I added a note in the wiki. Hopefully that helps out.

https://github.com/ddollar/foreman/wiki/Exporting-for-production

@x2es

This comment has been minimized.

Show comment
Hide comment
@x2es

x2es May 17, 2017

good to have foreman export -e HOME=/my/home (setup env right in command line)
which would be mixed with standard .env

x2es commented May 17, 2017

good to have foreman export -e HOME=/my/home (setup env right in command line)
which would be mixed with standard .env

@djch

This comment has been minimized.

Show comment
Hide comment
@djch

djch Aug 22, 2017

Hello — I'm hoping someone here can help me out. I'm using Foreman 0.84.0 on Debian 9.

The systemd config I'm exporting seems to ignore the environment configuration and fails with the same error as @wireframe: /bin/bash: line 0: exec: bundle: not found.

Export command:

sudo env "PATH=$PATH" foreman export systemd -a fishwife -u dan -m web=1,release=0,worker=0 /etc/systemd/system/

Resulting config file (/etc/systemd/system/fishwife-web@.service):

[Unit]
PartOf=fishwife-web.target

[Service]
User=dan
WorkingDirectory=/home/dan/fishwife
Environment=PORT=%i
Environment="PATH=/home/dan/.rbenv/plugins/ruby-build/bin:/home/dan/.rbenv/shims:/home/dan/.rbenv/bin:/home/dan/.rbenv/plugins/ruby-build/bin:/home/dan/.rbenv/shims:/home/dan/.rbenv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games"
Environment="RAILS_SERVE_STATIC_FILES=enabled"
Environment="RAILS_ENV=production"
Environment="RAILS_MASTER_KEY=redacted"
Environment="DATABASE_URL=redacted"
Environment="SHELL=/bin/bash"
Environment="HOME=/home/dan"
ExecStart=/bin/bash -lc 'bundle exec puma -C config/puma.rb'
Restart=always
StandardInput=null
StandardOutput=syslog
StandardError=syslog
SyslogIdentifier=%n
KillMode=mixed
TimeoutStopSec=5

Can anyone spot why all those Environment lines wouldn't be taken into account?

djch commented Aug 22, 2017

Hello — I'm hoping someone here can help me out. I'm using Foreman 0.84.0 on Debian 9.

The systemd config I'm exporting seems to ignore the environment configuration and fails with the same error as @wireframe: /bin/bash: line 0: exec: bundle: not found.

Export command:

sudo env "PATH=$PATH" foreman export systemd -a fishwife -u dan -m web=1,release=0,worker=0 /etc/systemd/system/

Resulting config file (/etc/systemd/system/fishwife-web@.service):

[Unit]
PartOf=fishwife-web.target

[Service]
User=dan
WorkingDirectory=/home/dan/fishwife
Environment=PORT=%i
Environment="PATH=/home/dan/.rbenv/plugins/ruby-build/bin:/home/dan/.rbenv/shims:/home/dan/.rbenv/bin:/home/dan/.rbenv/plugins/ruby-build/bin:/home/dan/.rbenv/shims:/home/dan/.rbenv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games"
Environment="RAILS_SERVE_STATIC_FILES=enabled"
Environment="RAILS_ENV=production"
Environment="RAILS_MASTER_KEY=redacted"
Environment="DATABASE_URL=redacted"
Environment="SHELL=/bin/bash"
Environment="HOME=/home/dan"
ExecStart=/bin/bash -lc 'bundle exec puma -C config/puma.rb'
Restart=always
StandardInput=null
StandardOutput=syslog
StandardError=syslog
SyslogIdentifier=%n
KillMode=mixed
TimeoutStopSec=5

Can anyone spot why all those Environment lines wouldn't be taken into account?

@djch

This comment has been minimized.

Show comment
Hide comment
@djch

djch Aug 22, 2017

Okay so, if I change ExecStart= to use /bin/bash -c instead of /bin/bash -lc, it works.

How come the generator adds -l to bash, and why wouldn't that that be compatible with the rest of my config?

(I think Foreman is rad and would really like to use the export function on this project if possible).

djch commented Aug 22, 2017

Okay so, if I change ExecStart= to use /bin/bash -c instead of /bin/bash -lc, it works.

How come the generator adds -l to bash, and why wouldn't that that be compatible with the rest of my config?

(I think Foreman is rad and would really like to use the export function on this project if possible).

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