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

Fix for chroot not getting supplemental groups #1398

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@coolacid
Contributor

coolacid commented May 22, 2014

linux chroot doesn't get the supplemental groups before dropping privileges. As such, we need to pull it from /etc/group and tweak it so that we can send them to chroot.

Original report came from @spuder on irc -- here is his GIST report:

https://gist.github.com/spuder/a1c3c7d10ce129507858

Spuder tested this and reported it working. Should work on other systems that use sysv init.

@coolacid

This comment has been minimized.

Show comment
Hide comment
@coolacid

coolacid May 22, 2014

Contributor

I could "fix" logstash-web, but I see no reason why it should require the extra groups to run.

Contributor

coolacid commented May 22, 2014

I could "fix" logstash-web, but I see no reason why it should require the extra groups to run.

@spuder

This comment has been minimized.

Show comment
Hide comment
@spuder

spuder May 22, 2014

Before the patch logstash could not read files owned by root:adm with permissions of 640, after the patch, logstach can read those files.

Tested on ubuntu 12.04

spuder commented May 22, 2014

Before the patch logstash could not read files owned by root:adm with permissions of 640, after the patch, logstach can read those files.

Tested on ubuntu 12.04

@electrical

This comment has been minimized.

Show comment
Hide comment
@electrical

electrical May 22, 2014

Contributor

Cool solution, im only afraid of the situation when there are no additional groups would the command fail then because its missing group names?

Perhaps an idea to do something like:
if CGROUPS is not empty then set EXTRA_GROUPS to '--groups $CGROUPS' and use the EXTRA_GROUPS in the command?

Contributor

electrical commented May 22, 2014

Cool solution, im only afraid of the situation when there are no additional groups would the command fail then because its missing group names?

Perhaps an idea to do something like:
if CGROUPS is not empty then set EXTRA_GROUPS to '--groups $CGROUPS' and use the EXTRA_GROUPS in the command?

Show outdated Hide outdated pkg/logstash.sysv
@coolacid

This comment has been minimized.

Show comment
Hide comment
@coolacid

coolacid May 22, 2014

Contributor

New commit tests to see if there was anything returned.

tested with my username - I get EXTRA_GROUPS, with another account it's blank as expected.

Contributor

coolacid commented May 22, 2014

New commit tests to see if there was anything returned.

tested with my username - I get EXTRA_GROUPS, with another account it's blank as expected.

@coolacid

This comment has been minimized.

Show comment
Hide comment
@coolacid

coolacid May 22, 2014

Contributor

I was not aware of the command - and you're right, pam could be a problem too.

The command as provided gives group numbers, which would work - let me play some more.

Contributor

coolacid commented May 22, 2014

I was not aware of the command - and you're right, pam could be a problem too.

The command as provided gives group numbers, which would work - let me play some more.

@coolacid

This comment has been minimized.

Show comment
Hide comment
@coolacid

coolacid May 22, 2014

Contributor

I don't remove the already set group anymore, but I can't see that hurting anything.

Contributor

coolacid commented May 22, 2014

I don't remove the already set group anymore, but I can't see that hurting anything.

@@ -50,11 +50,20 @@ start() {
JAVA_OPTS=${LS_JAVA_OPTS}
export PATH HOME JAVA_OPTS LS_HEAP_SIZE LS_JAVA_OPTS LS_USE_GC_LOGGING
# chown doesn't grab the suplimental groups when setting the user:group - so we have to do it for it.
# Boy, I hope we're root here.
SGROUPS=$(id -Gn "$LS_USER" | tr " " "," | sed 's/,$//'; echo '')

This comment has been minimized.

@jordansissel

jordansissel May 22, 2014

Contributor

+1 using id instead of grep /etc/group seems like a good approach.

@jordansissel

jordansissel May 22, 2014

Contributor

+1 using id instead of grep /etc/group seems like a good approach.

@electrical

This comment has been minimized.

Show comment
Hide comment
@electrical

electrical May 22, 2014

Contributor

👍 from me :-)

Contributor

electrical commented May 22, 2014

👍 from me :-)

@spuder

This comment has been minimized.

Show comment
Hide comment
@spuder

spuder May 22, 2014

The updated pull request works great for me.
74dfd2b

chmod 640 /tmp/foo
/etc/init.d/logstash-coolacid3 start
logstash started.
echo "logstash-coolacid3 640" >> /tmp/foo
tail -n1 /tmp/bar
{"message":"logstash-coolacid3 640","@version":"1","@timestamp":"2014-05-22T20:48:15.742Z","host":"foosball.ac","path":"/tmp/foo"}

If the init script was generated by 'pleaserun', I wonder if there needs to be a bug-hancement filed on that project as well?

spuder commented May 22, 2014

The updated pull request works great for me.
74dfd2b

chmod 640 /tmp/foo
/etc/init.d/logstash-coolacid3 start
logstash started.
echo "logstash-coolacid3 640" >> /tmp/foo
tail -n1 /tmp/bar
{"message":"logstash-coolacid3 640","@version":"1","@timestamp":"2014-05-22T20:48:15.742Z","host":"foosball.ac","path":"/tmp/foo"}

If the init script was generated by 'pleaserun', I wonder if there needs to be a bug-hancement filed on that project as well?

@electrical

This comment has been minimized.

Show comment
Hide comment
@electrical

electrical May 22, 2014

Contributor

@spuder all issues that we see here with with the init script we silently try to port over to pleaserun :-)
Feel free to open the issue there so we can solve it permanently.

Contributor

electrical commented May 22, 2014

@spuder all issues that we see here with with the init script we silently try to port over to pleaserun :-)
Feel free to open the issue there so we can solve it permanently.

@spuder

This comment has been minimized.

Show comment
Hide comment
@spuder

spuder Jun 3, 2014

Is there an expected release for this fix? I'd really like to see it make it into 1.4.2 since logstash is broken on all Ubuntu nodes at the moment.

spuder commented Jun 3, 2014

Is there an expected release for this fix? I'd really like to see it make it into 1.4.2 since logstash is broken on all Ubuntu nodes at the moment.

@victorgp

This comment has been minimized.

Show comment
Hide comment
@victorgp

victorgp Jul 10, 2014

How is possible that logstash is broken in, at least, all ubuntu nodes and this fix is not already merged?

victorgp commented Jul 10, 2014

How is possible that logstash is broken in, at least, all ubuntu nodes and this fix is not already merged?

@elasticsearch-release

This comment has been minimized.

Show comment
Hide comment
@elasticsearch-release

elasticsearch-release Jul 10, 2014

Can one of the admins verify this patch?

elasticsearch-release commented Jul 10, 2014

Can one of the admins verify this patch?

@jordansissel

This comment has been minimized.

Show comment
Hide comment
@jordansissel

jordansissel Jul 10, 2014

Contributor

@victorgp I am sorry that you are negatively impacted by this. From your comment, I can tell you are excited to see this patch merged!

While I believe this patch to work (testing by hand on one OS distro version), it would be greatly useful to us to know if it works for you. Further, this script is also used on CentOS/RHEL/SLES systems, can you help us test on those? I don't have access yet to all of those systems so it is difficult to do such testing in isolation. We are always working to improve our testing infrastructure as well to help cover these situations.

Any testing you can do on this would be lovely. As a bonus, by testing this, you would apply a workaround for your own infrastructure until we provide a release with this patch (or something like it that is known to work on all supported platforms).

I would hate to merge this patch only to have you or someone else find it in the next release having caused something else to break. This is why testing is so important to us.

This is an open source project and community, and we are stronger if we work together! Any energy you can provide is most appreciated, though obviously not required :)

Contributor

jordansissel commented Jul 10, 2014

@victorgp I am sorry that you are negatively impacted by this. From your comment, I can tell you are excited to see this patch merged!

While I believe this patch to work (testing by hand on one OS distro version), it would be greatly useful to us to know if it works for you. Further, this script is also used on CentOS/RHEL/SLES systems, can you help us test on those? I don't have access yet to all of those systems so it is difficult to do such testing in isolation. We are always working to improve our testing infrastructure as well to help cover these situations.

Any testing you can do on this would be lovely. As a bonus, by testing this, you would apply a workaround for your own infrastructure until we provide a release with this patch (or something like it that is known to work on all supported platforms).

I would hate to merge this patch only to have you or someone else find it in the next release having caused something else to break. This is why testing is so important to us.

This is an open source project and community, and we are stronger if we work together! Any energy you can provide is most appreciated, though obviously not required :)

@victorgp

This comment has been minimized.

Show comment
Hide comment
@victorgp

victorgp Jul 10, 2014

@jordansissel sorry for the negativity, i didn't want to seem so negative. I will try to help you testing the patch in Debian and Ubuntu, i don't have access to other distros either. Let's see if i can find some free time one of these days to test it.

victorgp commented Jul 10, 2014

@jordansissel sorry for the negativity, i didn't want to seem so negative. I will try to help you testing the patch in Debian and Ubuntu, i don't have access to other distros either. Let's see if i can find some free time one of these days to test it.

@victorgp

This comment has been minimized.

Show comment
Hide comment
@victorgp

victorgp Jul 15, 2014

The tests i've done tell me that this fix is not fully complete. It does work for old scripts located in /etc/init.d/ but it is still broken for upstart scripts in /etc/init/

I suppose the file logstash.upstart.ubuntu must be fixed, but it is no using chroot to run the program, why? i guess both should run in the same way...

BTW anyone knows what is the version that introduced this bug?

victorgp commented Jul 15, 2014

The tests i've done tell me that this fix is not fully complete. It does work for old scripts located in /etc/init.d/ but it is still broken for upstart scripts in /etc/init/

I suppose the file logstash.upstart.ubuntu must be fixed, but it is no using chroot to run the program, why? i guess both should run in the same way...

BTW anyone knows what is the version that introduced this bug?

@electrical

This comment has been minimized.

Show comment
Hide comment
@electrical

electrical Jul 15, 2014

Contributor

Hi,

The upstart script didn't get the love yet it deserves but we will soon do that.

Contributor

electrical commented Jul 15, 2014

Hi,

The upstart script didn't get the love yet it deserves but we will soon do that.

@spuder

This comment has been minimized.

Show comment
Hide comment
@spuder

spuder Jul 15, 2014

I find it very interesting that other people are still able to use logstash despite this bug. It is an absolute blocker for me. I'm trying to setup the simplest possible logstash cluster as shown in the examples in the logstash book, but can't work around this without significant chmod hackery.

Surely every one who tries to use logstash is running into this bug, no?
Maybe every other sysadmin is just doing a chmod 777 on all their logs.
If anyone has thoughts on why more people aren't blocked by this, I'd love to hear it.

spuder commented Jul 15, 2014

I find it very interesting that other people are still able to use logstash despite this bug. It is an absolute blocker for me. I'm trying to setup the simplest possible logstash cluster as shown in the examples in the logstash book, but can't work around this without significant chmod hackery.

Surely every one who tries to use logstash is running into this bug, no?
Maybe every other sysadmin is just doing a chmod 777 on all their logs.
If anyone has thoughts on why more people aren't blocked by this, I'd love to hear it.

@coolacid

This comment has been minimized.

Show comment
Hide comment
@coolacid

coolacid Jul 18, 2014

Contributor

As far as I can see upstart doesn't use chown -- and IIRC when I did these patches, I looked though all the startup scripts to see if they would have the same problem.

Contributor

coolacid commented Jul 18, 2014

As far as I can see upstart doesn't use chown -- and IIRC when I did these patches, I looked though all the startup scripts to see if they would have the same problem.

@heytchap

This comment has been minimized.

Show comment
Hide comment
@heytchap

heytchap Jul 23, 2014

Hey guys. Not sure if it's just me but the @coolacid modifications posted above still prevent me from reading anything root:adm with 640 permissions. Have modified both sysv and upstart but issue remains. Am I the only one?

heytchap commented Jul 23, 2014

Hey guys. Not sure if it's just me but the @coolacid modifications posted above still prevent me from reading anything root:adm with 640 permissions. Have modified both sysv and upstart but issue remains. Am I the only one?

@coolacid

This comment has been minimized.

Show comment
Hide comment
@coolacid

coolacid Jul 23, 2014

Contributor

@heytchap Did you add logstash to the adm group?

Contributor

coolacid commented Jul 23, 2014

@heytchap Did you add logstash to the adm group?

@heytchap

This comment has been minimized.

Show comment
Hide comment
@heytchap

heytchap Jul 23, 2014

@coolacid I did. I also added the LS user to root and syslog.
to confirm, the following command gives me this output:

id logstash
uid=999(logstash) gid=999(logstash) groups=999(logstash),0(root),4(adm),104(syslog)

so Im not sure what the hangup is here.

heytchap commented Jul 23, 2014

@coolacid I did. I also added the LS user to root and syslog.
to confirm, the following command gives me this output:

id logstash
uid=999(logstash) gid=999(logstash) groups=999(logstash),0(root),4(adm),104(syslog)

so Im not sure what the hangup is here.

@lasse-aagren

This comment has been minimized.

Show comment
Hide comment
@lasse-aagren

lasse-aagren Aug 27, 2014

setfacl is easy and works fine with logstash for these kind of files on Debian GNU/Linux (where I have tested it). It is just an abstraction layer that I would like to do without. Probably mostly out of old habit :) And it is easy to distribute with configuration management like puppet or similar, so if setfacl is the solution, I'll accept it :)

lasse-aagren commented Aug 27, 2014

setfacl is easy and works fine with logstash for these kind of files on Debian GNU/Linux (where I have tested it). It is just an abstraction layer that I would like to do without. Probably mostly out of old habit :) And it is easy to distribute with configuration management like puppet or similar, so if setfacl is the solution, I'll accept it :)

@jordansissel

This comment has been minimized.

Show comment
Hide comment
@jordansissel

jordansissel Aug 27, 2014

Contributor

@lasse-aagren Indeed! My hope, for now, is that setfacl is simply a workaround until we can fix it. This assumes all process-launcher systems are capable of setting supplementary groups- Supervisord, for example, appears only to support setting user, not even group, nor supplementary groups.

Contributor

jordansissel commented Aug 27, 2014

@lasse-aagren Indeed! My hope, for now, is that setfacl is simply a workaround until we can fix it. This assumes all process-launcher systems are capable of setting supplementary groups- Supervisord, for example, appears only to support setting user, not even group, nor supplementary groups.

@jordansissel jordansissel added the O(0) label Sep 22, 2014

@jordansissel jordansissel added the O(0) label Nov 11, 2014

@jacek-berlin

This comment has been minimized.

Show comment
Hide comment
@jacek-berlin

jacek-berlin Nov 28, 2014

Hi,
I've just stumbled upon this bug (Logstash 1.4.2, Debian Wheezy) .

Quick and dirty fix (if you trust your system) would be also to comment out the chroot line.

#### Works fine

# Run the program!
#  nice -n ${LS_NICE} chroot --userspec $LS_USER:$LS_GROUP / 

    sh -c "
    cd $LS_HOME
    ulimit -n ${LS_OPEN_FILES}
    exec \"$program\" $args
  " > "${LS_LOG_DIR}/$name.stdout" 2> "${LS_LOG_DIR}/$name.err" &

jacek-berlin commented Nov 28, 2014

Hi,
I've just stumbled upon this bug (Logstash 1.4.2, Debian Wheezy) .

Quick and dirty fix (if you trust your system) would be also to comment out the chroot line.

#### Works fine

# Run the program!
#  nice -n ${LS_NICE} chroot --userspec $LS_USER:$LS_GROUP / 

    sh -c "
    cd $LS_HOME
    ulimit -n ${LS_OPEN_FILES}
    exec \"$program\" $args
  " > "${LS_LOG_DIR}/$name.stdout" 2> "${LS_LOG_DIR}/$name.err" &
@sdklein

This comment has been minimized.

Show comment
Hide comment
@sdklein

sdklein Feb 12, 2015

+1 for the solution. I tested this pull request on CentOS 7 and it worked as expected.

sdklein commented Feb 12, 2015

+1 for the solution. I tested this pull request on CentOS 7 and it worked as expected.

@jordansissel jordansissel added the O(0) label Feb 25, 2015

@franklinwise

This comment has been minimized.

Show comment
Hide comment
@franklinwise

franklinwise Apr 28, 2015

When is this getting merged?

franklinwise commented Apr 28, 2015

When is this getting merged?

@suyograo suyograo removed the confirmed label May 15, 2015

@jhoff909

This comment has been minimized.

Show comment
Hide comment
@jhoff909

jhoff909 Jun 9, 2015

Curious when this is going to get merged? Like others, I can't imagine that most folks using logstash to read files aren't running into this and spending a lot of time either figuring it out or reading posts like the above...

jhoff909 commented Jun 9, 2015

Curious when this is going to get merged? Like others, I can't imagine that most folks using logstash to read files aren't running into this and spending a lot of time either figuring it out or reading posts like the above...

@suyograo suyograo added packaging v1.5.1 v1.5.2 and removed v1.5.1 labels Jun 9, 2015

@vasti

This comment has been minimized.

Show comment
Hide comment
@vasti

vasti Jun 16, 2015

Variable HOME is exported without setting value to it. There's HOME=${LS_HOME} on master.
I get error No SINCEDB_DIR or HOME environment variable set, I don't know where to keep track of the files I'm watching. Either set HOME or SINCEDB_DIR in your environment, or set sincedb_path in in your Logstash config for the file input with path because of that.

vasti commented Jun 16, 2015

Variable HOME is exported without setting value to it. There's HOME=${LS_HOME} on master.
I get error No SINCEDB_DIR or HOME environment variable set, I don't know where to keep track of the files I'm watching. Either set HOME or SINCEDB_DIR in your environment, or set sincedb_path in in your Logstash config for the file input with path because of that.

@jordansissel jordansissel removed the O(0) label Jun 29, 2015

@jordansissel jordansissel added v1.5.3 and removed v1.5.2 labels Jul 6, 2015

@jordansissel

This comment has been minimized.

Show comment
Hide comment
@jordansissel

jordansissel Jul 6, 2015

Contributor

I think we've had enough folks report that this patch works for them that I am ok to merge it.

Contributor

jordansissel commented Jul 6, 2015

I think we've had enough folks report that this patch works for them that I am ok to merge it.

@elasticsearch-bot

This comment has been minimized.

Show comment
Hide comment
@elasticsearch-bot

elasticsearch-bot Jul 6, 2015

Merged sucessfully into master 1.5!

elasticsearch-bot commented Jul 6, 2015

Merged sucessfully into master 1.5!

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