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

Improve documentation about enabling core dumps #6723

Closed
widhalmt opened this issue Oct 24, 2018 · 12 comments
Closed

Improve documentation about enabling core dumps #6723

widhalmt opened this issue Oct 24, 2018 · 12 comments
Labels
area/documentation End-user or developer help

Comments

@widhalmt
Copy link
Member

Hi,

While I enabled core dumps on some hosts of mine I realized that https://icinga.com/docs/icinga2/latest/doc/21-development/#core-dump might not be completely up to best practices for systemd.

I'll provide a PR with some things I found.

Is there any old documentation about core dumps on SysV hosts I'm missing. I suspect there's missing some information in the docs and I'd add this as well.

Cheers,
Thomas

widhalmt added a commit to widhalmt/icinga2 that referenced this issue Oct 24, 2018
@mcktr mcktr added the area/documentation End-user or developer help label Oct 24, 2018
@mphilipps
Copy link

With systemd core dumps are usually handled by systemd-coredump, as such the core_pattern should be |/lib/systemd/systemd-coredump %P %u %g %s %t %c %h %e also is fs.suid_dumpable = 1 really necessary for icinga2? At least on my systems here the setuid bit isn't set for Icinga2 and dumping setuid processed is a security risk.

@widhalmt
Copy link
Member Author

I could change that. Could any of the developers verify if there are any special requirements of Icinga 2 which would prohibit such a change?

@dgoetz
Copy link
Contributor

dgoetz commented Oct 30, 2018

I would not restrict to one pid as it is unlikely but possible that different process have different limits. So do something like for pid in $(pidof icinga2);do cat /proc/$pid/limits;done.

@mphilipps is right about Systemd, but it requires a specific version and is not available in CentOS 7 for example, so we should cover both. Also fs.suid_dumpable = 1 is not a good idea, 2 would be safe and only required if plugins run setuid and cause Icinga 2 to crash which is very unlikely, so I would leave it out.

@lazyfrosch
Copy link
Contributor

I commented on the PR

@widhalmt
Copy link
Member Author

I did a quick test (on CentOS 6):

# sysctl --system | grep suid
fs.suid_dumpable = 0
# for i in $(pgrep icinga2); do kill -SIGQUIT $i; done
# ls -lah /var/lib/cores/
total 8,0K
drwxrwsrwx.  2 root root 4,0K 30. Okt 12:15 .
drwxr-xr-x. 35 root root 4,0K 30. Okt 03:35 ..


# vim /etc/sysctl.conf 
# sysctl --system | grep suid
fs.suid_dumpable = 1
# for i in $(pgrep icinga2); do kill -SIGQUIT $i; done
# ls -lah /var/lib/cores/
total 19M
drwxrwsrwx.  2 root   root 4,0K 30. Okt 12:19 .
drwxr-xr-x. 35 root   root 4,0K 30. Okt 03:35 ..
-rw-------.  1 icinga root  29M 30. Okt 12:19 core.icinga2.25214


# vim /etc/sysctl.conf 
# sysctl --system | grep suid
fs.suid_dumpable = 2
# for i in $(pgrep icinga2); do kill -SIGQUIT $i; done
# ls -lah /var/lib/cores/
total 57M
drwxrwsrwx.  2 root   root 4,0K 30. Okt 12:20 .
drwxr-xr-x. 35 root   root 4,0K 30. Okt 03:35 ..
-rw-------.  1 icinga root  29M 30. Okt 12:19 core.icinga2.25214
-rw-------.  1 root root  29M 30. Okt 12:29 core.icinga2.27047

(The output was "tuned" a bit to improve clarity, so don't check timestamps etc.)

@dgoetz
Copy link
Contributor

dgoetz commented Oct 30, 2018

Ok, then I would opt for 2 as it does not allow access to the coredump to unprivileged users.

@dnsmichi
Copy link
Contributor

I'm currently rewriting the development chapter in a local branch. What's the status and common idea for the core dumps chapter joining this issue and the linked PR? If needed, I'd keep this specific for Systemd and non-Systemd stuff like seen everywhere else already.

@dnsmichi dnsmichi added the needs feedback We'll only proceed once we hear from you again label Nov 21, 2018
@dnsmichi
Copy link
Contributor

dnsmichi commented Dec 4, 2018

Since https://github.com/Icinga/icinga2/pull/6724/files#r229265113 mentions systemd-coredump I've played around with it on CentOS 7. The documentation is sparse, and I couldn't get any core dump to work with systemd-coredumpctl.

Instead, the default sysctl way of setting the coredump pattern and suid=2 thingy worked immediately.

I've found this old thread from 2014 https://lists.opensuse.org/opensuse-factory/2014-08/msg00478.html which describes the exact core_pattern for Systemd (which isn't described in their man pages).
/usr/lib/sysctl.d/50-coredump.conf doesn't exist either by default.

One can manually create it, or adjust the core_pattern.

sysctl -w kernel.core_pattern="/usr/lib/systemd/systemd-coredump %p %u %g %s %t %e"

This doesn't work though, there's no file on disk even with coredump.conf Storage=External.

Imho this is complicated as fuck. I would stick to our sysctl defaults in case of emergency, which can be explained and given to users and customers.

@lazyfrosch
Copy link
Contributor

@dnsmichi systemd-coredump would be preferred for newer versions of systemd, the version in CentOS is quite old...

Main problem is currently that multiple limits apply:

systemd.exec -> /usr/sbin/icinga2 -> bash -> limits.conf -> /usr/libXX/icinga2/sbin/icinga2

@dnsmichi
Copy link
Contributor

dnsmichi commented Dec 4, 2018

Having multiple methods with different Systemd versions likely irritates the user.

I'm leaving the doc parts "as is" with some smaller stolen fixes for edits and suid bits. My branch solely brings the development docs into shape, so the linked PR needs to be rebased later when this is merged.

dnsmichi pushed a commit that referenced this issue Dec 4, 2018
The main parts are discussed in #6723
@lippserd
Copy link
Member

lippserd commented Feb 6, 2019

Hi everyone,

How do we proceed here?

Best,
Eric

dnsmichi pushed a commit that referenced this issue Feb 11, 2019
The main parts are discussed in #6723

(cherry picked from commit 43c1710)
@dnsmichi dnsmichi assigned widhalmt and unassigned dnsmichi Mar 11, 2019
@dnsmichi
Copy link
Contributor

dnsmichi commented Apr 8, 2019

Hi,

I'd say you'll create a PR with the changes, and if there's still something required, do a review or an offline meeting.

I'll close here.

Cheers,
Michael

@dnsmichi dnsmichi closed this as completed Apr 8, 2019
@dnsmichi dnsmichi removed the needs feedback We'll only proceed once we hear from you again label Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation End-user or developer help
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants