Use sudo for privileged process checks on filedescriptors #715

pdecat · 2017-08-29T16:16:56Z

What does this PR do?

Repackaged the patch I suggested at DataDog/dd-agent#2033 (comment) in a dd-agent-omnibus compatible way (no additional python file, only process/check.py is modified).

Motivation

Datadog does not provide out of box support for monitoring file descriptors used by processes not running under the same user (dd-agent by default).

The suggested work-around of running the datadog agent as root is explicitly not recommended which is probably reasonable: https://help.datadoghq.com/hc/en-us/articles/115000066506-Why-don-t-I-see-the-system-processes-open-file-descriptors-metric-

Additional Notes

This requires the addition of the following sudo policies in the deb and rpm packages:

dd-agent ALL=NOPASSWD: /opt/datadog-agent/embedded/bin/python /opt/datadog-agent/agent/checks.d/process.py num_fds *
dd-agent ALL=NOPASSWD: /opt/datadog-agent/embedded/bin/python /opt/datadog-agent/agent/checks.d/process.pyc num_fds *

Usage of sudo generates frequent logging in /var/log/auth.log. That may be silenced with additional PAM rules in /etc/pam.d/sudo if desired.

Resolves DataDog/dd-agent/issues#2033

The same may be done for the io_counters method.

bits-bot · 2017-08-29T16:16:58Z

@pdecat, thanks for your PR! By analyzing the history of the files in this pull request, we identified @masci and @truthbk to be potential reviewers.

gmmeyer · 2017-10-26T22:03:00Z

Hey @pdecat! Thanks a lot for your contribution!

This is a good idea but I am not sure that it should be turned on by default in the package. I am wary about giving our agent any sudo power that it doesn't absolutely need. So I don't think this should be turned on by default in the packages. Can you add instructions to turn it on in the readme and in the case of failure after attempting to use sudo.

Thanks a lot, again, for your contribution! 😄

pdecat · 2017-10-26T22:20:41Z

Hi @gmmeyer , I'll document the sudoers part in the README.

Also, what would you think about adding an option to the process check's configuration to enable the use of sudo, disabled by default? That would avoid failed sudo executions in case the sudoers part is not set up.

gmmeyer · 2017-10-31T23:22:05Z

I think that's very wise, it shouldn't fail when it doesn't have the proper permissions. It should log something on info but not fail.

gmmeyer

Hey @pdecat! Thanks again for this PR!

This is a cool solution. I have two comments, and after you resolve them in one way or another I'm pretty happy to merge.

gmmeyer · 2017-12-18T18:33:48Z

process/check.py

+            if method == 'num_fds' and Platform.is_unix():
+                try:
+                    # It is up the agent's packager to grant corresponding sudo policy on unix platforms
+                    result = int(subprocess.check_output(['sudo', sys.executable, __file__, method, str(process.pid)]))


This is a cool solution but I am not certain it will work with Agent6, where we're calling the python library from within golang. At least for now, you'd have to ensure this happens only in Agent5. You can import the version from here: https://github.com/DataDog/dd-agent/blob/master/config.py#L44.

@masci how do you think we would be able to do this in Agent6?

As a second note here, why did you choose to execute this file rather than including that as a string that would be passed to the interpreter? I think that's more likely to work in Agent6 than this is.

Hi @gmmeyer, I chose to invoke this script on purpose for two reasons:

For security reasons: passing the process id to this script requires wildcards. Knowing that this script can only accept a process id and not anything else feels safe to me.
Passing an arbitrary string to an interpreter with a wildcard has dangerous implications: https://blog.compass-security.com/2012/10/dangerous-sudoers-entries-part-4-wildcards/
An alternative may be to have several sudoers entries to match all possible pid lengths:

dd-agent ALL=NOPASSWD: /opt/datadog-agent/embedded/bin/python -c "import psutil; import sys; psutil.num_fds(sys.argv[1])" [1-9] dd-agent ALL=NOPASSWD: /opt/datadog-agent/embedded/bin/python -c "import psutil; import sys; psutil.num_fds(sys.argv[1])" [1-9][0-9] dd-agent ALL=NOPASSWD: /opt/datadog-agent/embedded/bin/python -c "import psutil; import sys; psutil.num_fds(sys.argv[1])" [1-9][0-9][0-9] dd-agent ALL=NOPASSWD: /opt/datadog-agent/embedded/bin/python -c "import psutil; import sys; psutil.num_fds(sys.argv[1])" [1-9][0-9][0-9][0-9] dd-agent ALL=NOPASSWD: /opt/datadog-agent/embedded/bin/python -c "import psutil; import sys; psutil.num_fds(sys.argv[1])" [1-6][0-5][0-9][0-9][0-9]

For easy distribution: as the omnibus packaging rename files, this solution should work everywhere, no need to know the path to the script.

Okay, so that makes sense. However, in Agent v6 we're not using a python interpreter, though, it's embedded within a go application. I think (I haven't verified this) that this would just call the go application instead of the python interpreter.

I tested github.com/sbinet/go-python that is used in agent 6 to wrap the python interpreter and it seems to be ok:

main.go:

package main import ( "fmt" "log" "github.com/sbinet/go-python" ) func init() { err := python.Initialize() if err != nil { log.Panic(err.Error()) } } func main() { module := python.PyImport_ImportModule("testsys") if module == nil { log.Fatal("could not import 'testsys'") } name := module.GetAttrString("__name__") if name == nil { log.Fatal("could not getattr '__name__'") } defer name.DecRef() fmt.Printf("testsys.__name__: %q\n", python.PyString_AsString(name)) testsys := module.GetAttrString("testsys") if testsys == nil { log.Fatal("could not getattr 'testsys'") } defer testsys.DecRef() o1 := testsys.CallFunction() if o1 == nil { log.Fatal("could not call 'testsys.testsys()'") } fmt.Printf("%s\n", python.PyString_AsString(o1)) o1.DecRef() }

testsys.py:

#!/usr/bin/env python2 import sys def testsys(): return "sys.executable=" + sys.executable

Result:

# PYTHONPATH=.:$PYTHONPATH go run ./main.go testsys.__name__: "testsys" sys.executable=/usr/bin/python

I'm down to merge this then! I'll approve it!

Can you rebase it and then I'll approve, sorry for taking so long on this 😄

No prob, rebased!

However, I did not yet implement the part about adding an option to the process check's configuration to enable the use of sudo, disabled by default.

…ataDog/dd-agent/issues#2033)

pdecat · 2018-01-18T11:34:47Z

Added try_sudo setting with some documentation.

…with psutil (`num_fds`)

gmmeyer

🎉 we're gonna merge!

pdecat · 2018-02-01T19:25:24Z

Thanks! 🎉

…)" This reverts commit e1f1d53. Check is not sending the open_file_descriptor metric. Reverting for 5.22.0 release.

pdecat · 2018-02-14T12:50:31Z

Hi @hush-hush / @ChristineTChen, I get that this was reverted for 5.22.

What issues did you have?

Did you enable the sudo part to actually get open_file_descriptor metrics?

jippi · 2018-03-12T12:40:03Z

Bump ^ @hush-hush @ChristineTChen

pdecat · 2018-03-12T13:00:09Z

Re-submitted this PR as #1235

jeremy-lq added the community label Aug 30, 2017

hush-hush assigned truthbk and gmmeyer Sep 4, 2017

masci unassigned truthbk Oct 27, 2017

pdecat mentioned this pull request Nov 9, 2017

Get metrics regarding opened file handlers DataDog/dd-agent#853

Closed

gmmeyer reviewed Dec 18, 2017

View reviewed changes

Use sudo for privileged process checks on filedescriptors (resolves D…

ee2b57a

…ataDog/dd-agent/issues#2033)

Add try_sudo setting to try using sudo to collect open_fd metric …

33159d8

…with psutil (`num_fds`)

gmmeyer approved these changes Feb 1, 2018

View reviewed changes

gmmeyer added this to the 5.22 milestone Feb 1, 2018

zippolyte merged commit e1f1d53 into DataDog:master Feb 1, 2018

zippolyte mentioned this pull request Feb 1, 2018

[process] Update changelog and version for process check #1077

Merged

4 tasks

pdecat deleted the f-process-check-with-sudo branch February 1, 2018 19:24

ChristineTChen added a commit that referenced this pull request Feb 9, 2018

Revert "Use sudo for privileged process checks on filedescriptors (#715…

247e987

…)" This reverts commit e1f1d53. Check is not sending the open_file_descriptor metric. Reverting for 5.22.0 release.

ChristineTChen added a commit that referenced this pull request Feb 9, 2018

Revert "Use sudo for privileged process checks on filedescriptors (#715…

fbaad1b

…)" This reverts commit e1f1d53. Check is not sending the open_file_descriptor metric. Reverting for 5.22.0 release.

hush-hush pushed a commit that referenced this pull request Feb 9, 2018

Revert "Use sudo for privileged process checks on filedescriptors (#715…

4ead9e3

…)" This reverts commit e1f1d53. Check is not sending the open_file_descriptor metric. Reverting for 5.22.0 release.

pdecat restored the f-process-check-with-sudo branch February 14, 2018 12:48

pdecat mentioned this pull request Mar 12, 2018

Use sudo for privileged process checks on filedescriptors (take 2) #1235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use sudo for privileged process checks on filedescriptors #715

Use sudo for privileged process checks on filedescriptors #715

pdecat commented Aug 29, 2017 •

edited

Loading

bits-bot commented Aug 29, 2017

gmmeyer commented Oct 26, 2017

pdecat commented Oct 26, 2017 •

edited

Loading

gmmeyer commented Oct 31, 2017

gmmeyer left a comment

gmmeyer Dec 18, 2017

gmmeyer Dec 18, 2017

pdecat Dec 19, 2017 •

edited

Loading

gmmeyer Dec 22, 2017

pdecat Dec 22, 2017

gmmeyer Jan 17, 2018

gmmeyer Jan 17, 2018

pdecat Jan 18, 2018

pdecat commented Jan 18, 2018

gmmeyer left a comment

pdecat commented Feb 1, 2018 •

edited

Loading

pdecat commented Feb 14, 2018

jippi commented Mar 12, 2018

pdecat commented Mar 12, 2018

Use sudo for privileged process checks on filedescriptors #715

Use sudo for privileged process checks on filedescriptors #715

Conversation

pdecat commented Aug 29, 2017 • edited Loading

What does this PR do?

Motivation

Additional Notes

bits-bot commented Aug 29, 2017

gmmeyer commented Oct 26, 2017

pdecat commented Oct 26, 2017 • edited Loading

gmmeyer commented Oct 31, 2017

gmmeyer left a comment

Choose a reason for hiding this comment

gmmeyer Dec 18, 2017

Choose a reason for hiding this comment

gmmeyer Dec 18, 2017

Choose a reason for hiding this comment

pdecat Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

gmmeyer Dec 22, 2017

Choose a reason for hiding this comment

pdecat Dec 22, 2017

Choose a reason for hiding this comment

gmmeyer Jan 17, 2018

Choose a reason for hiding this comment

gmmeyer Jan 17, 2018

Choose a reason for hiding this comment

pdecat Jan 18, 2018

Choose a reason for hiding this comment

pdecat commented Jan 18, 2018

gmmeyer left a comment

Choose a reason for hiding this comment

pdecat commented Feb 1, 2018 • edited Loading

pdecat commented Feb 14, 2018

jippi commented Mar 12, 2018

pdecat commented Mar 12, 2018

pdecat commented Aug 29, 2017 •

edited

Loading

pdecat commented Oct 26, 2017 •

edited

Loading

pdecat Dec 19, 2017 •

edited

Loading

pdecat commented Feb 1, 2018 •

edited

Loading